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

On Mon, Feb 3, 2025 at 12:36 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Sun, Feb 2, 2025 at 9: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.
>
> Thank you for reviewing the 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>.
> > ```
>
> Agreed, will fix.
>
> >
> > 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.
> > ```
>
> But this function neither connects to nor disconnects from databases, either.
>
> >
> > 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.
>
> I'm concerned that it reduces the usability; users (or existing
> extensions) would need to construct the whole connection string just
> to pass the database name. If we want to avoid exposing
> GetDbnameFromConnectionOptions(), I'd introduce another exposed
> function for that, say GenerateRecoveryConfigWithConnStr().

I've attached the updated patch. I address all comments I got so far
and added a small regression test.

It makes sense to me that we move GetDbnameFromConnectionOptions() to
recovery_gen.c since this function is currently used only with
GenerateRecoveryConfig() which is defined in the same file. If we find
a more appropriate place, we can move it later. Feedback is very
welcome.

Regards,

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