Re: Replace pg_controldata output fields with macros for better code manageability

At Wed, 2 Feb 2022 18:02:39 -0500, Bruce Momjian <bruce(at)momjian(dot)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