Обсуждение: Replace pg_controldata output fields with macros for better code manageability

Hi,

While working on another pg_control patch, I observed that the
pg_controldata output fields such as "Latest checkpoint's
TimeLineID:", "Latest checkpoint's NextOID:'' and so on, are being
used in pg_resetwal.c, pg_controldata.c and pg_upgrade/controldata.c.
Direct usage of these fields in many places doesn't look good and can
be error prone if we try to change the text in one place and forget in
another place. I'm thinking of having those fields as macros in
pg_control.h, something like [1] and use it all the places. This will
be a good idea for better code manageability IMO.

Thoughts?

[1]
#define PG_CONTROL_FIELD_VERSION_NUMBER "pg_control version number:"
#define PG_CONTROL_FIELD_CATALOG_NUMBER "Catalog version number:"
#define PG_CONTROL_FIELD_CHECKPOINT_TLI "Latest checkpoint's TimeLineID:"
#define PG_CONTROL_FIELD_CHECKPOINT_PREV_TLI "Latest checkpoint's
PrevTimeLineID:"
#define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID "Latest checkpoint's oldestXID:"
#define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID_DB "Latest checkpoint's
oldestXID's DB:"
and so on.

Regards,
Bharath Rupireddy.

On Sat, Jan 29, 2022 at 08:00:53PM +0530, Bharath Rupireddy wrote:
> Hi,
> 
> While working on another pg_control patch, I observed that the
> pg_controldata output fields such as "Latest checkpoint's
> TimeLineID:", "Latest checkpoint's NextOID:'' and so on, are being
> used in pg_resetwal.c, pg_controldata.c and pg_upgrade/controldata.c.
> Direct usage of these fields in many places doesn't look good and can
> be error prone if we try to change the text in one place and forget in
> another place. I'm thinking of having those fields as macros in
> pg_control.h, something like [1] and use it all the places. This will
> be a good idea for better code manageability IMO.
> 
> Thoughts?
> 
> [1]
> #define PG_CONTROL_FIELD_VERSION_NUMBER "pg_control version number:"
> #define PG_CONTROL_FIELD_CATALOG_NUMBER "Catalog version number:"
> #define PG_CONTROL_FIELD_CHECKPOINT_TLI "Latest checkpoint's TimeLineID:"
> #define PG_CONTROL_FIELD_CHECKPOINT_PREV_TLI "Latest checkpoint's
> PrevTimeLineID:"
> #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID "Latest checkpoint's oldestXID:"
> #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID_DB "Latest checkpoint's
> oldestXID's DB:"
> and so on.

That seems like a very good idea.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

At Wed, 2 Feb 2022 18:02:39 -0500, Bruce Momjian <bruce@momjian.us> wrote in 
> On Sat, Jan 29, 2022 at 08:00:53PM +0530, Bharath Rupireddy wrote:
> > Hi,
> > 
> > While working on another pg_control patch, I observed that the
> > pg_controldata output fields such as "Latest checkpoint's
> > TimeLineID:", "Latest checkpoint's NextOID:'' and so on, are being
> > used in pg_resetwal.c, pg_controldata.c and pg_upgrade/controldata.c.
> > Direct usage of these fields in many places doesn't look good and can
> > be error prone if we try to change the text in one place and forget in
> > another place. I'm thinking of having those fields as macros in
> > pg_control.h, something like [1] and use it all the places. This will
> > be a good idea for better code manageability IMO.
> > 
> > Thoughts?
> > 
> > [1]
> > #define PG_CONTROL_FIELD_VERSION_NUMBER "pg_control version number:"
> > #define PG_CONTROL_FIELD_CATALOG_NUMBER "Catalog version number:"
> > #define PG_CONTROL_FIELD_CHECKPOINT_TLI "Latest checkpoint's TimeLineID:"
> > #define PG_CONTROL_FIELD_CHECKPOINT_PREV_TLI "Latest checkpoint's
> > PrevTimeLineID:"
> > #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID "Latest checkpoint's oldestXID:"
> > #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID_DB "Latest checkpoint's
> > oldestXID's DB:"
> > and so on.
> 
> That seems like a very good idea.

+1 for consolidate the texts an any shape.

But it doesn't sound like the way we should take to simply replace
only the concrete text labels to symbols.  Significant downsides of
doing that are untranslatability and difficulty to add the proper
indentation before the value field.  So we need to define the texts
including indentation spaces and format placeholder.

But if we define the strings separately, I would rather define them in
an array.



typedef enum pg_control_fields
{
  PGCNTRL_FIELD_ERROR = -1,
  PGCNTRL_FIELD_VERSION = 0,
  PGCNTRL_FIELD_CATALOG_VERSION,
  ...
  PGCTRL_NUM_FIELDS
} pg_control_fields;

const char *pg_control_item_formats[] = {
    gettext_noop("pg_control version number:            %u\n"),
    gettext_noop("Catalog version number:               %u\n"),
    ...
};

const char *
get_pg_control_item_format(pg_control_fields item_id)
{
    return _(pg_control_item_formats[item_id]);
};

const char *
get_pg_control_item()
{
    return _(pg_control_item_formats[item_id]);
};

pg_control_fields
get_pg_control_item(const char *str, const char **valp)
{
    int i;
    for (i = 0 ; i < PGCNTL_NUM_FIELDS ; i++)
    {
        const char *fmt = pg_control_item_formats[i];
        const char *p = strchr(fmt, ':');

        Assert(p);
        if (strncmp(str, fmt, p - fmt) == 0)
        {
            p = str + (p - fmt);
            for(p++ ; *p == ' ' ; p++);
            *valp = p;
            return i;
        }
    }

    return -1;
}

Printer side does:

   printf(get_pg_control_item_format(PGCNTRL_FIELD_VERSION),
          ControlFile->pg_control_version);
       
And the reader side would do:

   switch(get_pg_control_item(str, &v))
   {
       case PGCNTRL_FIELD_VERSION:
           cluster->controldata.ctrl_ver = str2uint(v);
           break;
   ...

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

      

On Thu, Feb 3, 2022 at 11:34 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> > > #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID "Latest checkpoint's oldestXID:"
> > > #define PG_CONTROL_FIELD_CHECKPOINT_OLDESTXID_DB "Latest checkpoint's
> > > oldestXID's DB:"
> > > and so on.
> >
> > That seems like a very good idea.
>
> +1 for consolidate the texts an any shape.
>
> But it doesn't sound like the way we should take to simply replace
> only the concrete text labels to symbols.  Significant downsides of
> doing that are untranslatability and difficulty to add the proper
> indentation before the value field.  So we need to define the texts
> including indentation spaces and format placeholder.

It looks like we also translate the printf statements that tools emit.
I'm not sure how having a macro in the printf creates a problem with
the translation.

Yes, the indentation part needs to be taken care in any case, which is
also true now, different fields are adjusted to different indentation
(number of spaces, not tabs) for the sake of readability in the
output.

> But if we define the strings separately, I would rather define them in
> an array.
>
> typedef enum pg_control_fields
> {
>
> const char *pg_control_item_formats[] = {
>         gettext_noop("pg_control version number:            %u\n"),
>
> const char *
> get_pg_control_item_format(pg_control_fields item_id)
> {
>         return _(pg_control_item_formats[item_id]);
> };
>
> const char *
> get_pg_control_item()
> {
>         return _(pg_control_item_formats[item_id]);
> };
>
> pg_control_fields
> get_pg_control_item(const char *str, const char **valp)
> {
>         int i;
>         for (i = 0 ; i < PGCNTL_NUM_FIELDS ; i++)
>         {
>                 const char *fmt = pg_control_item_formats[i];
>                 const char *p = strchr(fmt, ':');
>
>                 Assert(p);
>                 if (strncmp(str, fmt, p - fmt) == 0)
>                 {
>                         p = str + (p - fmt);
>                         for(p++ ; *p == ' ' ; p++);
>                         *valp = p;
>                         return i;
>                 }
>         }
>
>         return -1;
> }
>
> Printer side does:
>
>    printf(get_pg_control_item_format(PGCNTRL_FIELD_VERSION),
>               ControlFile->pg_control_version);
>
> And the reader side would do:
>
>    switch(get_pg_control_item(str, &v))
>    {
>        case PGCNTRL_FIELD_VERSION:
>            cluster->controldata.ctrl_ver = str2uint(v);
>                    break;

Thanks for your time on the above code. IMHO, I don't know if we ever
go the complex way with the above sort of style (error-prone, huge
maintenance burden and extra function calls). Instead, I would be
happy to keep the code as is if the macro-way(as proposed originally
in this thread) really has a translation problem.

Regards,
Bharath Rupireddy.

On 29.01.22 15:30, Bharath Rupireddy wrote:
> While working on another pg_control patch, I observed that the
> pg_controldata output fields such as "Latest checkpoint's
> TimeLineID:", "Latest checkpoint's NextOID:'' and so on, are being
> used in pg_resetwal.c, pg_controldata.c and pg_upgrade/controldata.c.

The duplication between pg_resetwal and pg_controldata could probably be 
handled by refactoring the code so that only one place is responsible 
for printing it.  The usages in pg_upgrade are probably best guarded by 
tests that ensure that any changes anywhere else don't break things. 
Using macros like you suggest only protects against one kind of change: 
changing the wording of a line.  But it doesn't protect for example 
against a line being removed from the output.

While I'm sympathetic to the issue you describe, the proposed solution 
introduces a significant amount of ugliness for a problem that is really 
quite rare.