Subject: Re: Have pg_basebackup write "dbname" in "primary_conninfo"?

From: Robert Haas <[email-masked]>
Date: Tue, 20 Feb 2024 19:56:10 +0530

On Tue, Feb 20, 2024 at 5:58 PM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> > Seems weird to me. You don't use dbname=replication to ask for a
> > replication connection, so why would we ever end up with that
> > anywhere? And especially in only one of two such closely related
> > cases?
>
> Just FYI - here is an extreme case. And note that I have applied proposed patch.
>
> When `pg_basebackup -D data_N2 -R` is used:
> ```
> primary_conninfo = 'user=hayato ... dbname=hayato ...
> ```
>
> But when `pg_basebackup -d "" -D data_N2 -R` is used:
> ```
> primary_conninfo = 'user=hayato ... dbname=replication
> ```

It seems like maybe somebody should look into why this is happening,
and perhaps fix it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 21 Feb 2024 12:04:24 +0900 (JST)

At Tue, 20 Feb 2024 19:56:10 +0530, Robert Haas <[email-masked]> wrote in 
> It seems like maybe somebody should look into why this is happening,

GetConnection()@streamutil.c wants to ensure conninfo has a fallback
database name ("replication"). However, the function seems to be
ignoring the case where neither dbname nor connection string is given,
which is the first case Kuroda-san raised. The second case is the
intended behavior of the function.

>	/* pg_recvlogical uses dbname only; others use connection_string only. */
>	Assert(dbname == NULL || connection_string == NULL);

And the function incorrectly assumes that the connection string
requires "dbname=replication".

>	 * Merge the connection info inputs given in form of connection string,
>	 * options and default values (dbname=replication, replication=true, etc.)

But the name is a pseudo database name only used by pg_hba.conf
(maybe) , which cannot be used as an actual database name (without
quotation marks, or unless it is actually created). The function
should not add the fallback database name because the connection
string for physical replication connection doesn't require the dbname
parameter. (attached patch)

About the proposed patch, pg_basebackup cannot verify the validity of
the dbname. It could be problematic.

Although I haven't looked the original thread, it seems that the
dbname is used only by pg_sync_replication_slots(). If it is true,
couldn't we make the SQL function require a database name to make a
connection, instead of requiring it in physical-replication conninfo?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 56d1b15951..da63a04de4 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -78,7 +78,7 @@ GetConnection(void)
 
 	/*
 	 * Merge the connection info inputs given in form of connection string,
-	 * options and default values (dbname=replication, replication=true, etc.)
+	 * options and default values (replication=true, etc.)
 	 */
 	i = 0;
 	if (connection_string)
@@ -96,14 +96,6 @@ GetConnection(void)
 		keywords = pg_malloc0((argcount + 1) * sizeof(*keywords));
 		values = pg_malloc0((argcount + 1) * sizeof(*values));
 
-		/*
-		 * Set dbname here already, so it can be overridden by a dbname in the
-		 * connection string.
-		 */
-		keywords[i] = "dbname";
-		values[i] = "replication";
-		i++;
-
 		for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
 		{
 			if (conn_opt->val != NULL && conn_opt->val[0] != '\0')

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 21 Feb 2024 14:37:21 +1100

> connection, instead of requiring it in physical-replication conninfo?

I agree. If the intention is to meet the new requirement of the sync-slot
patch which requires a dbname in the primary_conninfo, then pseudo dbnames
will not work, whether it be the username or just "replication". I feel if
the user does not specify dbname explicitly in pg_basebackup it should be
left blank in the generated primary_conninfo string as well.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 21 Feb 2024 14:41:59 +1100

> connection, instead of requiring it in physical-replication conninfo?
In the original thread, the intention is to not just provide this
functionality  using the function pg_sync_replication_slots(), but provide
a GUC option on standbys to sync logical replication slots periodically
even without calling that function. This requires connecting to a database.

regards,
Ajin Cherian
Fujitsu Australia

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 21 Feb 2024 05:08:51 +0000

Dear Horiguchi-san,

> parameter. (attached patch)

I was also missing, but the requirement was that the dbname should be included
only when the dbname option was explicitly specified [1]. Even mine and yours
cannot handle like that. Libpq function PQconnectdbParams()->pqConnectOptions2()
fills all the parameter to PGconn, at that time the information whether it is
intentionally specified or not is discarded. Then, GenerateRecoveryConfig() would
just write down all the connection parameters from PGconn, they cannot recognize.

One approach is that based on Horiguchi-san's one and initial patch, we can
avoid writing when the dbname is same as the username.

[1]: https://www.postgresql.org/message-id/CAA4eK1KH1d1J5giPMZVOtMe0iqncf1CpNwkBKoYAmXdC-kEGZg%40mail.gmail.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 21 Feb 2024 16:28:36 +1100

On Wed, Feb 21, 2024 at 4:09 PM Hayato Kuroda (Fujitsu) <
[email-masked]> wrote:

> > >     /* pg_recvlogical uses dbname only; others use connection_string
> only.
> > */
> > >      * options and default values (dbname=replication,
> replication=true,
> > etc.)
> >
> Well, one option is that if a default dbname should be written to the
configuration file, then "postgres' is a better option than "replication"
or "username" as the default option, at least a db of that name exists.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Fri, 23 Feb 2024 12:17:07 +0530

> parameter. (attached patch)

We do append dbname=replication even in libpqrcv_connect for .pgpass
lookup as mentioned in comments. See below:
libpqrcv_connect()
{
....
else
{
/*
* The database name is ignored by the server in replication mode,
* but specify "replication" for .pgpass lookup.
*/
keys[++i] = "dbname";
vals[i] = "replication";
}
...
}

I think as part of this effort we should just add dbname to
primary_conninfo written in postgresql.auto.conf file. As above, the
question is valid whether we should do it just for 17 or for prior
versions. Let's discuss more on that. I am not sure of the use case
for versions before 17 but commit cca97ce6a665 mentioned that some
middleware or proxies might however need to know the dbname to make
the correct routing decision for the connection. Does that apply here
as well? If so, we can do it and update the docs, otherwise, let's do
it just for 17.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 27 Feb 2024 08:30:19 +0000

Dear Amit,

> * but specify "replication" for .pgpass lookup.

OK. So we must add the value for the authorization, right?
I think it should be described even in GetConnection().

> as well? If so, we can do it and update the docs, otherwise, let's do

Hmm, I might lose your requirements again. So, we must satisfy all of below
ones, right?
1) add {"dbname", "replication"} key-value pair to look up .pgpass file correctly.
2) If the -R is given, output dbname=xxx value to be used by slotsync worker.
3) If the dbname is not given in the connection string, the same string as
   username must be used to keep the libpq connection rule.
4) No need to add dbname=replication pair 

PSA the patch for implementing it. It is basically same as Ian's one.
However, this patch still cannot satisfy the condition 3).

`pg_basebackup -D data_N2 -d "user=postgres" -R`
-> dbname would not be appeared in primary_conninfo.

This is because `if (connection_string)` case in GetConnection() explicy override
a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair
before the overriding, but it is no-op. Because The replacement of the dbname in
pqConnectOptions2() would be done only for the valid (=lastly specified)
connection options.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 27 Feb 2024 08:37:34 +0000

> pqConnectOptions2() would be done only for the valid (=lastly specified)

Oh, this patch missed the straightforward case:

pg_basebackup -D data_N2 -d "user=postgres dbname=replication" -R
-> dbname would not be appeared in primary_conninfo.

So I think it cannot be applied as-is. Sorry for sharing the bad item.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED
https://www.fujitsu.com/

---

From: Amit Kapila <[email-masked]>
Date: Mon, 11 Mar 2024 17:15:57 +0530

> So I think it cannot be applied as-is. Sorry for sharing the bad item.

Can you please share the patch that can be considered for review?

-- 
With Regards,
Amit Kapila.

---

From: vignesh C <[email-masked]>
Date: Tue, 12 Mar 2024 17:12:57 +0530

> Can you please share the patch that can be considered for review?

Here is a patch with few changes: a) Removed the version check based
on discussion from [1] b) updated the documentation.
I have tested various scenarios with the attached patch, the dbname
that is written in postgresql.auto.conf for each of the cases with
pg_basebackup is given below:
1) ./pg_basebackup -U vignesh -R
-> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo
will have dbname as username specified)
2) ./pg_basebackup -D data -R
-> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo
will have the dbname as username (which is the same as the OS user
while setting defaults))
3) ./pg_basebackup -d "user=vignesh"  -D data -R
-> primary_conninfo = "dbname=replication"  (In this case
primary_conninfo will have dbname as replication which is the default
value from GetConnection as connection string is specified)
4) ./pg_basebackup -d "user=vignesh dbname=postgres"  -D data -R
-> primary_conninfo = "dbname=postgres" (In this case primary_conninfo
will have the dbname as the dbname specified)
5) ./pg_basebackup -d ""  -D data -R
-> primary_conninfo = "dbname=replication" (In this case
primary_conninfo will have dbname as replication which is the default
value from GetConnection as connection string is specified)

I have mentioned the reasons as to why dbname is being set to
replication or username in each of the cases. How about replacing
these values in postgresql.auto.conf manually.

[1] - https://www.postgresql.org/message-id/CAGECzQTh9oB3nu98DsHMpRaVaqXPDRgTDEikY82OAKYF0%3DhVMA%40mail.gmail.com

Regards,
Vignesh
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 88c689e725..1a6980d1b0 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -243,7 +243,10 @@ PostgreSQL documentation
         The <filename>postgresql.auto.conf</filename> file will record the connection
         settings and, if specified, the replication slot
         that <application>pg_basebackup</application> is using, so that
-        streaming replication will use the same settings later on.
+        a) synchronization of logical replication slots on the primary to the
+        hot_standby from <link linkend="pg-sync-replication-slots">
+        <function>pg_sync_replication_slots</function></link> or slot sync worker
+        and b) streaming replication will use the same settings later on.
        </para>
 
       </listitem>
diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c
index 2585f11939..cb37703ab9 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -49,7 +49,6 @@ 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'))

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 Mar 2024 16:58:09 +0530

On Tue, Mar 12, 2024 at 5:13 PM vignesh C <[email-masked]> wrote:
> replication or username in each of the cases.
>

IIUC, the dbname is already allowed in connstring for pg_basebackup by
commit cca97ce6a6 and with this patch we are just writing it in
postgresql.auto.conf if -R option is used to write recovery info. This
will help slot syncworker to automatically connect with database
without user manually specifying the dbname and replication
connection, the same will still be ignored. I don't see any problem
with this. Does anyone else see any problem?

+        <function>pg_sync_replication_slots</function></link> or slot
sync worker

We can simply modify the last line as: ".. streaming replication or
logical replication slots synchronization will use the same settings
later on." Additionally, you can give a link reference to [1].

[1] - https://www.postgresql.org/docs/devel/logicaldecoding-explanation.html#LOGICALDECODING-REPLICATION-SLOTS-SYNCHRONIZATION
-- 
With Regards,
Amit Kapila.