Horton

Thread

Commits

  1. pg_rewind: Add dbname to primary_conninfo when using --write-recovery-conf.

  2. Allow dbname to be written as part of connstring via pg_basebackup's -R option.

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-01-29T21:47:08Z

    Hi,

I found that pg_rewind with --write-recovery-conf option doesn't write
the dbname to an auto-generated primary_conninfo value. Therefore,
after a failover, the old primary cannot start if it's rewound by
pg_rewind with --write-recovery-conf option if sync_replication_slots
is on. Is it an oversight in commit a145f424d5? I could not see any
discussion in the thread[1] about whether we need to support adding
the dbname to the primary_conninfo value generated by pg_rewind.

I think it's a good idea to support it at least on HEAD. I've attached
a patch for that.

Regards,

[1] https://www.postgresql.org/message-id/CAB8KJ%3DhdKdg%2BUeXhReeHpHA6N6v3e0qFF%2BZsPFHk9_ThWKf%3D2A%40mail.gmail.com

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

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

    Amit Kapila <amit.kapila16@gmail.com> — 2025-02-03T03:30:47Z

    On Thu, Jan 30, 2025 at 3:17 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I found that pg_rewind with --write-recovery-conf option doesn't write
> the dbname to an auto-generated primary_conninfo value. Therefore,
> after a failover, the old primary cannot start if it's rewound by
> pg_rewind with --write-recovery-conf option if sync_replication_slots
> is on. Is it an oversight in commit a145f424d5?
>

IIRC, we tried to do minimal in the first version as we didn't have
all the use cases. However, it makes sense to support this in HEAD as
proposed by you.

-- 
With Regards,
Amit Kapila.

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-02-03T05:50:32Z

    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

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

    Peter Smith <smithpb2250@gmail.com> — 2025-02-03T05:59:37Z

    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

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-02-03T19:32:28Z

    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

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-02-03T20:36:21Z

    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

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-02-11T23:56:04Z

    On Mon, Feb 3, 2025 at 12:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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().

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

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

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-02-12T05:36:01Z

    Dear Sawada-san,

Thanks for updating the patch!

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

I considered your idea that adding new API, but it seemed for me to have less
benefit. Also, I do not know better place for the declaration now. Overall, the
patch looks good to me.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-03-11T23:26:15Z

    On Tue, Feb 11, 2025 at 9:36 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Sawada-san,
>
> Thanks for updating the patch!
>
> > 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.
>
> I considered your idea that adding new API, but it seemed for me to have less
> benefit. Also, I do not know better place for the declaration now. Overall, the
> patch looks good to me.
>

I'm going to push the v2 patch, barring any objections and further comments.


Regards,

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

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

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-03-13T01:05:02Z

    On Tue, Mar 11, 2025 at 4:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 9:36 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Sawada-san,
> >
> > Thanks for updating the patch!
> >
> > > 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.
> >
> > I considered your idea that adding new API, but it seemed for me to have less
> > benefit. Also, I do not know better place for the declaration now. Overall, the
> > patch looks good to me.
> >
>
> I'm going to push the v2 patch, barring any objections and further comments.

Pushed.

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