pg_rewind with --write-recovery-conf option doesn't write dbname to primary_conninfo value.
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]/messages/by-id/CAB8KJ=hdKdg+UeXhReeHpHA6N6v3e0qFF+ZsPFHk9_ThWKf=2A@mail.gmail.com 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]: /messages/by-id/CAB8KJ=hdKdg+UeXhReeHpHA6N6v3e0qFF+ZsPFHk9_ThWKf=2A@mail.gmail.com
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-pg_rewind-Add-dbname-to-primary_connifo-with-writ.patchapplication/octet-stream; name=v1-0001-pg_rewind-Add-dbname-to-primary_connifo-with-writ.patchDownload
From 22c4bcdde3f1c7f7d3122299f9aaf408a38f19f2 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 29 Jan 2025 11:34:55 -0800
Subject: [PATCH v1] pg_rewind: Add dbname to primary_connifo with
--write-recovery-conf.
Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch-through:
---
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/streamutil.c | 69 ---------------------------
src/bin/pg_basebackup/streamutil.h | 2 -
src/bin/pg_rewind/pg_rewind.c | 6 ++-
src/fe_utils/recovery_gen.c | 65 +++++++++++++++++++++++++
src/include/fe_utils/recovery_gen.h | 1 +
6 files changed, 71 insertions(+), 74 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dc0c805137a..d4b4e334014 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1818,7 +1818,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
if (writerecoveryconf)
recoveryconfcontents = GenerateRecoveryConfig(conn,
replication_slot,
- GetDbnameFromConnectionOptions());
+ GetDbnameFromConnectionOptions(connection_string));
/*
* Run IDENTIFY_SYSTEM so we can get the timeline
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 021ab61fcb0..8e605f43ffe 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -32,7 +32,6 @@
int WalSegSz;
static bool RetrieveDataDirCreatePerm(PGconn *conn);
-static char *FindDbnameInConnParams(PQconninfoOption *conn_opts);
/* SHOW command for replication connection was introduced in version 10 */
#define MINIMUM_VERSION_FOR_SHOW_CMD 100000
@@ -269,74 +268,6 @@ GetConnection(void)
return tmpconn;
}
-/*
- * FindDbnameInConnParams
- *
- * This is a helper function for GetDbnameFromConnectionOptions(). Extract
- * the value of dbname from PQconninfoOption parameters, if it's present.
- * Returns a strdup'd result or NULL.
- */
-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;
-}
-
-/*
- * GetDbnameFromConnectionOptions
- *
- * This is a special purpose function to retrieve the dbname from either the
- * connection_string specified by the user or from the environment variables.
- *
- * We follow GetConnection() to fetch the dbname from various connection
- * options.
- *
- * Returns NULL, if dbname is not specified by the user in the above
- * mentioned connection options.
- */
-char *
-GetDbnameFromConnectionOptions(void)
-{
- PQconninfoOption *conn_opts;
- char *err_msg = NULL;
- char *dbname;
-
- /* First try to get the dbname from connection string. */
- if (connection_string)
- {
- conn_opts = PQconninfoParse(connection_string, &err_msg);
- if (conn_opts == NULL)
- pg_fatal("%s", err_msg);
-
- dbname = FindDbnameInConnParams(conn_opts);
-
- PQconninfoFree(conn_opts);
- if (dbname)
- return dbname;
- }
-
- /*
- * Next try to get the dbname from default values that are available from
- * the environment.
- */
- conn_opts = PQconndefaults();
- if (conn_opts == NULL)
- pg_fatal("out of memory");
-
- dbname = FindDbnameInConnParams(conn_opts);
-
- PQconninfoFree(conn_opts);
- return dbname;
-}
-
/*
* From version 10, explicitly set wal segment size using SHOW wal_segment_size
* since ControlFile is not accessible here.
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index 472193e239f..f917c43517f 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -31,8 +31,6 @@ extern PGconn *conn;
extern PGconn *GetConnection(void);
-extern char *GetDbnameFromConnectionOptions(void);
-
/* Replication commands */
extern bool CreateReplicationSlot(PGconn *conn, const char *slot_name,
const char *plugin, bool is_temporary,
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index cae81cd6cb1..fbd29d81068 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -451,7 +451,8 @@ main(int argc, char **argv)
pg_log_info("no rewind required");
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL, NULL));
+ GenerateRecoveryConfig(conn, NULL,
+ GetDbnameFromConnectionOptions(connstr_source)));
exit(0);
}
@@ -528,7 +529,8 @@ main(int argc, char **argv)
/* Also update the standby configuration, if requested. */
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL, NULL));
+ GenerateRecoveryConfig(conn, NULL,
+ GetDbnameFromConnectionOptions(connstr_source)));
/* don't need the source connection anymore */
source->destroy(source);
diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c
index 7c172f65a10..eaf2c125c16 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -168,3 +168,68 @@ escape_quotes(const char *src)
pg_fatal("out of memory");
return result;
}
+
+/*
+ * FindDbnameInConnParams
+ *
+ * This is a helper function for GetDbnameFromConnectionOptions(). Extract
+ * the value of dbname from PQconninfoOption parameters, if it's present.
+ * Returns a strdup'd result or NULL.
+ */
+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;
+}
+
+/*
+ * 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.
+ */
+char *
+GetDbnameFromConnectionOptions(const char *connstr)
+{
+ PQconninfoOption *conn_opts;
+ char *err_msg = NULL;
+ char *dbname;
+
+ /* First try to get the dbname from connection string. */
+ if (connstr)
+ {
+ conn_opts = PQconninfoParse(connstr, &err_msg);
+ if (conn_opts == NULL)
+ pg_fatal("%s", err_msg);
+
+ dbname = FindDbnameInConnParams(conn_opts);
+
+ PQconninfoFree(conn_opts);
+ if (dbname)
+ return dbname;
+ }
+
+ /*
+ * Next try to get the dbname from default values that are available from
+ * the environment.
+ */
+ conn_opts = PQconndefaults();
+ if (conn_opts == NULL)
+ pg_fatal("out of memory");
+
+ dbname = FindDbnameInConnParams(conn_opts);
+
+ PQconninfoFree(conn_opts);
+ return dbname;
+}
diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h
index 6412ffdaffa..c13f2263bcd 100644
--- a/src/include/fe_utils/recovery_gen.h
+++ b/src/include/fe_utils/recovery_gen.h
@@ -25,5 +25,6 @@ extern PQExpBuffer GenerateRecoveryConfig(PGconn *pgconn,
char *dbname);
extern void WriteRecoveryConfig(PGconn *pgconn, const char *target_dir,
PQExpBuffer contents);
+extern char *GetDbnameFromConnectionOptions(const char *connstr);
#endif /* RECOVERY_GEN_H */
--
2.43.5
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.
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
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
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 LIMITEDHi 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 messageno details.
bad link.
Yeah, I cannot add the discussion link before sending the patch.
======
src/fe_utils/recovery_gen.c1.
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
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
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
Attachments:
v2-0001-pg_rewind-Add-dbname-to-primary_conninfo-when-usi.patchapplication/octet-stream; name=v2-0001-pg_rewind-Add-dbname-to-primary_conninfo-when-usi.patchDownload
From 81378de0e494c9299b29d9c1573f10c24153b255 Mon Sep 17 00:00:00 2001
From: Masahiko Sawada <sawada.mshk@gmail.com>
Date: Wed, 29 Jan 2025 11:34:55 -0800
Subject: [PATCH v2] pg_rewind: Add dbname to primary_conninfo when using
--write-recovery-conf
This commit enhances pg_rewind's --write-recovery-conf option to include
the dbname in the generated primary_conninfo value when specified in the
--source-server option. With this modification, the rewound server can
connect to the primary server without manual configuration file
modifications when sync_replication_slots is enabled.
Reviewed-by: Hayato Kuroda, Peter Smith
Discussion: https://postgr.es/m/CAD21AoAkW=Ht0k9dVoBTCcqLiiZ2MXhVr+d=j2T_EZMerGrLWQ@mail.gmail.com
---
doc/src/sgml/ref/pg_rewind.sgml | 6 ++-
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/streamutil.c | 69 ---------------------------
src/bin/pg_basebackup/streamutil.h | 2 -
src/bin/pg_rewind/pg_rewind.c | 6 ++-
src/bin/pg_rewind/t/RewindTest.pm | 5 ++
src/fe_utils/recovery_gen.c | 66 +++++++++++++++++++++++++
src/include/fe_utils/recovery_gen.h | 1 +
8 files changed, 81 insertions(+), 76 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index dc039d87566..5485033ed8c 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -186,8 +186,10 @@ PostgreSQL documentation
<para>
Create <filename>standby.signal</filename> and append connection
settings to <filename>postgresql.auto.conf</filename> in the output
- directory. <literal>--source-server</literal> is mandatory with
- this option.
+ directory. The dbname will be recorded only if the dbname was
+ specified explicitly in the connection string or <link linkend="libpq-envars">
+ environment variable</link>. <literal>--source-server</literal> is
+ mandatory with this option.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index dc0c805137a..d4b4e334014 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1818,7 +1818,7 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
if (writerecoveryconf)
recoveryconfcontents = GenerateRecoveryConfig(conn,
replication_slot,
- GetDbnameFromConnectionOptions());
+ GetDbnameFromConnectionOptions(connection_string));
/*
* Run IDENTIFY_SYSTEM so we can get the timeline
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 021ab61fcb0..8e605f43ffe 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -32,7 +32,6 @@
int WalSegSz;
static bool RetrieveDataDirCreatePerm(PGconn *conn);
-static char *FindDbnameInConnParams(PQconninfoOption *conn_opts);
/* SHOW command for replication connection was introduced in version 10 */
#define MINIMUM_VERSION_FOR_SHOW_CMD 100000
@@ -269,74 +268,6 @@ GetConnection(void)
return tmpconn;
}
-/*
- * FindDbnameInConnParams
- *
- * This is a helper function for GetDbnameFromConnectionOptions(). Extract
- * the value of dbname from PQconninfoOption parameters, if it's present.
- * Returns a strdup'd result or NULL.
- */
-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;
-}
-
-/*
- * GetDbnameFromConnectionOptions
- *
- * This is a special purpose function to retrieve the dbname from either the
- * connection_string specified by the user or from the environment variables.
- *
- * We follow GetConnection() to fetch the dbname from various connection
- * options.
- *
- * Returns NULL, if dbname is not specified by the user in the above
- * mentioned connection options.
- */
-char *
-GetDbnameFromConnectionOptions(void)
-{
- PQconninfoOption *conn_opts;
- char *err_msg = NULL;
- char *dbname;
-
- /* First try to get the dbname from connection string. */
- if (connection_string)
- {
- conn_opts = PQconninfoParse(connection_string, &err_msg);
- if (conn_opts == NULL)
- pg_fatal("%s", err_msg);
-
- dbname = FindDbnameInConnParams(conn_opts);
-
- PQconninfoFree(conn_opts);
- if (dbname)
- return dbname;
- }
-
- /*
- * Next try to get the dbname from default values that are available from
- * the environment.
- */
- conn_opts = PQconndefaults();
- if (conn_opts == NULL)
- pg_fatal("out of memory");
-
- dbname = FindDbnameInConnParams(conn_opts);
-
- PQconninfoFree(conn_opts);
- return dbname;
-}
-
/*
* From version 10, explicitly set wal segment size using SHOW wal_segment_size
* since ControlFile is not accessible here.
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index 472193e239f..f917c43517f 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -31,8 +31,6 @@ extern PGconn *conn;
extern PGconn *GetConnection(void);
-extern char *GetDbnameFromConnectionOptions(void);
-
/* Replication commands */
extern bool CreateReplicationSlot(PGconn *conn, const char *slot_name,
const char *plugin, bool is_temporary,
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index cae81cd6cb1..fbd29d81068 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -451,7 +451,8 @@ main(int argc, char **argv)
pg_log_info("no rewind required");
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL, NULL));
+ GenerateRecoveryConfig(conn, NULL,
+ GetDbnameFromConnectionOptions(connstr_source)));
exit(0);
}
@@ -528,7 +529,8 @@ main(int argc, char **argv)
/* Also update the standby configuration, if requested. */
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL, NULL));
+ GenerateRecoveryConfig(conn, NULL,
+ GetDbnameFromConnectionOptions(connstr_source)));
/* don't need the source connection anymore */
source->destroy(source);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 6115ec21eb9..ec3b4a51995 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -279,6 +279,11 @@ sub run_pg_rewind
],
'pg_rewind remote');
+ # Check that pg_rewind with dbname and --write-recovery-conf
+ # wrote the dbname in the generated primary_conninfo value.
+ like(slurp_file("$primary_pgdata/postgresql.auto.conf"),
+ qr/dbname=postgres/m, 'recovery conf file sets dbname');
+
# Check that standby.signal is here as recovery configuration
# was requested.
ok( -e "$primary_pgdata/standby.signal",
diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c
index 7c172f65a10..e9023584768 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -14,6 +14,7 @@
#include "fe_utils/string_utils.h"
static char *escape_quotes(const char *src);
+static char *FindDbnameInConnOpts(PQconninfoOption *conn_opts);
/*
* Write recovery configuration contents into a fresh PQExpBuffer, and
@@ -168,3 +169,68 @@ escape_quotes(const char *src)
pg_fatal("out of memory");
return result;
}
+
+/*
+ * FindDbnameInConnOpts
+ *
+ * This is a helper function for GetDbnameFromConnectionOptions(). Extract
+ * the value of dbname from PQconninfoOption parameters, if it's present.
+ * Returns a strdup'd result or NULL.
+ */
+static char *
+FindDbnameInConnOpts(PQconninfoOption *conn_opts)
+{
+ for (PQconninfoOption *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;
+}
+
+/*
+ * 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 given
+ * connection options.
+ */
+char *
+GetDbnameFromConnectionOptions(const char *connstr)
+{
+ PQconninfoOption *conn_opts;
+ char *err_msg = NULL;
+ char *dbname;
+
+ /* First try to get the dbname from connection string. */
+ if (connstr)
+ {
+ conn_opts = PQconninfoParse(connstr, &err_msg);
+ if (conn_opts == NULL)
+ pg_fatal("%s", err_msg);
+
+ dbname = FindDbnameInConnOpts(conn_opts);
+
+ PQconninfoFree(conn_opts);
+ if (dbname)
+ return dbname;
+ }
+
+ /*
+ * Next try to get the dbname from default values that are available from
+ * the environment.
+ */
+ conn_opts = PQconndefaults();
+ if (conn_opts == NULL)
+ pg_fatal("out of memory");
+
+ dbname = FindDbnameInConnOpts(conn_opts);
+
+ PQconninfoFree(conn_opts);
+ return dbname;
+}
diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h
index 6412ffdaffa..c13f2263bcd 100644
--- a/src/include/fe_utils/recovery_gen.h
+++ b/src/include/fe_utils/recovery_gen.h
@@ -25,5 +25,6 @@ extern PQExpBuffer GenerateRecoveryConfig(PGconn *pgconn,
char *dbname);
extern void WriteRecoveryConfig(PGconn *pgconn, const char *target_dir,
PQExpBuffer contents);
+extern char *GetDbnameFromConnectionOptions(const char *connstr);
#endif /* RECOVERY_GEN_H */
--
2.43.5
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
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
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