public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ian Lawrence Barwick <[email protected]>
To: pgsql-hackers <[email protected]>
Subject: Have pg_basebackup write "dbname" in "primary_conninfo"?
Date: Tue, 20 Feb 2024 08:34:22 +0900
Message-ID: <CAB8KJ=hdKdg+UeXhReeHpHA6N6v3e0qFF+ZsPFHk9_ThWKf=2A@mail.gmail.com> (raw)

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-STANDB...
[2] https://www.postgresql.org/docs/devel/runtime-config-replication.html#RUNTIME-CONFIG-REPLICATION-STA...

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


Attachments:

  [text/x-patch] pg_basebackup-write-dbname.v0001.patch (1.5K, ../CAB8KJ=hdKdg+UeXhReeHpHA6N6v3e0qFF+ZsPFHk9_ThWKf=2A@mail.gmail.com/2-pg_basebackup-write-dbname.v0001.patch)
  download | inline diff:
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,

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected]
  Subject: Re: Have pg_basebackup write "dbname" in "primary_conninfo"?
  In-Reply-To: <CAB8KJ=hdKdg+UeXhReeHpHA6N6v3e0qFF+ZsPFHk9_ThWKf=2A@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox