Hi
Hi
With the addition of "pg_sync_replication_slots()", there is now a use-case for
including "dbname" in "primary_conninfo" and the docs have changed from
stating [1]:
Do not specify a database name in the primary_conninfo string.
to [2]:
For replication slot synchronization (see Section 48.2.3), it is also
necessary to specify a valid dbname in the primary_conninfo string. This will
only be used for slot synchronization. It is ignored for streaming.
[1]
https://www.postgresql.org/docs/16/runtime-config-replication.html#RUNTIME-CONFIG-REPLICATION-STANDBY
[2]
https://www.postgresql.org/docs/devel/runtime-config-replication.html#RUNTIME-CONFIG-REPLICATION-STANDBY
However, when setting up a standby (with the intent of creating a logical
standby) with pg_basebackup, providing the -R/-write-recovery-conf option
results in a "primary_conninfo" string being generated without a "dbname"
parameter, which requires a manual change should one intend to use
"pg_sync_replication_slots()".
I can't see any reason for continuing to omit "dbname", so suggest it should
only continue to be omitted for 16 and earlier; see attached patch.
Note that this does mean that if the conninfo string provided to pg_basebackup
does not contain "dbname", the generated "primary_conninfo" GUC will default to
"dbname=replication". It would be easy enough to suppress this, but AFAICS
there's no way to tell if it was explicitly supplied by the user, in which case
it would be surprising if it were omitted from the final "primary_conninfo"
string.
The only other place where GenerateRecoveryConfig() is called is pg_rewind;
I can't see any adverse affects from the proposed change. But it's perfectly
possible there's something blindingly obvious I'm overlooking.
Regards
Ian Barwick
diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c
index 2585f11939..de463f3956 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -49,12 +49,16 @@ GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot)
{
/* Omit empty settings and those libpqwalreceiver overrides. */
if (strcmp(opt->keyword, "replication") == 0 ||
- strcmp(opt->keyword, "dbname") == 0 ||
strcmp(opt->keyword, "fallback_application_name") == 0 ||
(opt->val == NULL) ||
(opt->val != NULL && opt->val[0] == '\0'))
continue;
+ /* Omit "dbname" in PostgreSQL 16 and earlier. */
+ if (strcmp(opt->keyword, "dbname") == 0 &&
+ PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_DBNAME_PARAM)
+ continue;
+
/* Separate key-value pairs with spaces */
if (conninfo_buf.len != 0)
appendPQExpBufferChar(&conninfo_buf, ' ');
diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h
index ca2c4800d0..740d90c9d8 100644
--- a/src/include/fe_utils/recovery_gen.h
+++ b/src/include/fe_utils/recovery_gen.h
@@ -20,6 +20,12 @@
*/
#define MINIMUM_VERSION_FOR_RECOVERY_GUC 120000
+/*
+ * from version 17, there is a use-case for adding the dbname parameter
+ * to the generated "primary_conninfo" value
+ */
+#define MINIMUM_VERSION_FOR_DBNAME_PARAM 170000
+
extern PQExpBuffer GenerateRecoveryConfig(PGconn *pgconn,
char *replication_slot);
extern void WriteRecoveryConfig(PGconn *pgconn, char *target_dir,
