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

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

From

"Hayato Kuroda (Fujitsu)"

Date:

03 February, 11:50:32

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

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

From

Peter Smith

Date:

03 February, 11:59:37

On Mon, Feb 3, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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.

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

======
Commit message

no details.
bad link.

======
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.

~~~

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"

~

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

~~~

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.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

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

From

Masahiko Sawada

Date:

04 February, 01:32:28

On Sun, Feb 2, 2025 at 10:00 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Feb 3, 2025 at 4:50 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.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

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

From

Masahiko Sawada

Date:

04 February, 02:36:21

On Sun, Feb 2, 2025 at 9:50 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.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().

Regards,


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