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

On Thu, Feb 3, 2022 at 11:34 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)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.