Re: pg_rewind with --write-recovery-conf option doesn't write dbname to primary_conninfo value.

On Sun, Feb 2, 2025 at 10:00 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Mon, Feb 3, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > Dear Sawada-san,
> >
> > > I think it's a good idea to support it at least on HEAD. I've attached
> > > a patch for that.
> >
> > +1. I've confirmed that pg_rewind and -R can't output dbname for now,
> > and your patch allows to do it.
> >
> > Few comments for your patch.
> >
> > 1.
> >
> > pg_basebackup.sgml has below description. I feel this can be ported to
> > pg_rewind.sgml as well.
> >
> > ```
> > The dbname will be recorded only if the dbname was
> > specified explicitly in the connection string or <link linkend="libpq-envars">
> > environment variable</link>.
> > ```
> >
> > 2.
> > I'm not sure whether recovery_gen.h/c is a correct place for the exported function
> > GetDbnameFromConnectionOptions(). The function itself does not handle
> > postgresql.cuto.conf file.
> > I seeked other header files and felt that connect_utils.h might be.
> >
> > ```
> > /*-------------------------------------------------------------------------
> > *
> > * Facilities for frontend code to connect to and disconnect from databases.
> > ```
> >
> > Another idea is to change the third API to accept the whole connection string, and
> > it extracts dbname from it. In this approach we can make GetDbnameFromConnectionOptions()
> > to static function - which does not feel strange for me.
> >
> > Best regards,
> > Hayato Kuroda
> > FUJITSU LIMITED
> >
>
> Hi Sawada-san, h
>
> Here are a few more minor comments in addition to what Kuroda-San
> already posted.

Thank you for reviewing the patch.

>
> ======
> typo in patch name /primary_conninfo/primary_connifo/

Will fix.

>
> ======
> Commit message
>
> no details.
> bad link.

Yeah, I cannot add the discussion link before sending the patch.

>
> ======
> src/fe_utils/recovery_gen.c
>
> 1.
> static char *
> FindDbnameInConnParams(PQconninfoOption *conn_opts)
>
> There is a missing forward declaration of this function. Better to add
> it for consistency because the other static function has one.

Will fix.

>
> ~~~
>
> 2.
> +static char *
> +FindDbnameInConnParams(PQconninfoOption *conn_opts)
> +{
> + PQconninfoOption *conn_opt;
> +
> + for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
> + {
> + if (strcmp(conn_opt->keyword, "dbname") == 0 &&
> + conn_opt->val != NULL && conn_opt->val[0] != '\0')
> + return pg_strdup(conn_opt->val);
> + }
> + return NULL;
> +}
>
> 2a.
> I know you copied this, but it seems a bit strange that this function
> is named "Params" when everything about it including its parameter and
> comments and the caller talks about "_opts" or "Options"

Yes, it seems clearer to use ConnOpts instead.

>
> ~
>
> 2b.
> I know you copied this, but normally 'conn_opt' might have been
> written as a for-loop variable.

Fill fix.

>
> ~~~
>
> 3.
> +/*
> + * GetDbnameFromConnectionOptions
> + *
> + * This is a special purpose function to retrieve the dbname from either the
> + * 'connstr' specified by the caller or from the environment variables.
> + *
> + * Returns NULL, if dbname is not specified by the user in the above
> + * mentioned connection options.
> + */
>
> What does "in the above mentioned connection options" mean? In the
> original function comment where this was copied from there was an
> extra sentence ("We follow ... from various connection options.") so
> this had more context, but now that the other sentence is removed
> maybe "above mentioned connection options" looks like it also needs
> rewording.

Agreed, will fix.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com