On Thu, 14 Mar 2024 at 15:49, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 1:45 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 2:27 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 5:57 AM Masahiko Sawada <sawada.m...@gmail.com>
> > > wrote:
> > > >
> > > > This fact makes me think that the slotsync worker might be able to
> > > > accept the primary_conninfo value even if there is no dbname in the
> > > > value. That is, if there is no dbname in the primary_conninfo, it uses
> > > > the username in accordance with the specs of the connection string.
> > > > Currently, the slotsync worker connects to the local database first
> > > > and then establishes the connection to the primary server. But if we
> > > > can reverse the two steps, it can get the dbname that has actually
> > > > been used to establish the remote connection and use it for the local
> > > > connection too. That way, the primary_conninfo generated by
> > > > pg_basebackup could work even without the patch. For example, if the
> > > > OS user executing pg_basebackup is 'postgres', the slotsync worker
> > > > would connect to the postgres database. Given the 'postgres' database
> > > > is created by default and 'postgres' OS user is used in common, I
> > > > guess it could cover many cases in practice actually.
> > > >
> > >
> > > I think this is worth investigating but I suspect that in most cases
> > > users will end up using a replication connection without specifying
> > > the user name and we may not be able to give a meaningful error
> > > message when slotsync worker won't be able to connect. The same will
> > > be true even when the dbname same as the username would be used.
> >
> > What do you mean by not being able to give a meaningful error message?
> >
> > If the slotsync worker uses the user name as the dbname, and such a
> > database doesn't exist, the error message the user will get is
> > "database "test_user" does not exist". ISTM the same is true when the
> > user specifies the wrong database in the primary_conninfo.
> >
>
> Right, the exact error message as mentioned by Shveta will be:
> ERROR: could not connect to the primary server: connection to server
> at "127.0.0.1", port 5433 failed: FATAL: database "bckp_user" does not
> exist
>
> Now, without this idea, the ERROR message will be:
> ERROR: slot synchronization requires dbname to be specified in
> primary_conninfo
>
> I am not sure how much this matters but the second message sounds more useful.
>
> > >
> > > > Having said that, even with (or without) the above change, we might
> > > > want to change the pg_basebackup so that it writes the dbname to the
> > > > primary_conninfo if -R option is specified. Since the database where
> > > > the slotsync worker connects cannot be dropped while the slotsync
> > > > worker is running, the user might want to change the database to
> > > > connect, and it would be useful if they can do that using
> > > > pg_basebackup instead of modifying the configuration file manually.
> > > >
> > > > While the current approach makes sense to me, I'm a bit concerned that
> > > > we might end up having the pg_basebackup search the actual database
> > > > name (e.g. 'dbname=template1') from the .pgpass file instead of
> > > > 'dbname=replication'. As far as I tested on my environment, suppose
> > > > that I execute:
> > > >
> > > > pg_basebackup -D tmp -d "dbname=testdb" -R
> > > >
> > > > The pg_basebackup established a replication connection but looked for
> > > > the password of the 'testdb' database. This could be another
> > > > inconvenience for the existing users who want to use the slot
> > > > synchronization.
> > > >
> > >
> > > This is true because it is internally using logical replication
> > > connection (as we will set set replication=database).
> >
> > Did you mean the pg_basebackup is using a logical replication
> > connection in this case? As far as I tested, even if we specify dbname
> > to the -d option of pg_basebackup, it uses a physical replication
> > connection. For example, it can take a backup even if I specify a
> > non-existing database name.
> >
>
> You are right. I misunderstood some part of the code in GetConnection.
> However, I think my point is still valid that if the user has provided
> dbname in the connection string it means that she wants that database
> entry to be looked upon not "replication" entry.
>
> >
> > >
> > > > But it's still just an idea and I might be missing something. And
> > > > given we're getting closer to the feature freeze, it would be a PG18
> > > > item.
> > > >
> > >
> > > +1. At this stage, it is important to discuss whether we should allow
> > > pg_baseback to write dbname (either a specified one or a default one)
> > > along with other parameters in primary_conninfo?
> > >
> >
> > True. While I basically agree that pg_basebackup writes dbname in
> > primary_conninfo, I'm concerned that writing "dbname=replication"
> > could be problematic. Quoting the case 3) Vignesh summarized before:
> >
> > 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)
> >
> > The primary_conninfo generated by pg_basebackup -R is now used by
> > either a walreceiver (for physical replication connection) or a
> > slotsync worker (for normal connection). The "dbname=replication" is
> > okay for walreceiver. On the other hand, as for the slotsync worker,
> > it can pass the CheckAndGetDbnameFromConninfo() check but it's very
> > likely that it cannot connect to the primary since most users won't
> > create a database with "replication" name. The user will end up
> > getting an error message like 'database "replication" does not exist'
> > but I'm not sure it would be informative for users. Rather, the error
> > message "slot synchronization requires dbname to be specified in
> > primary_conninfo" might be more informative for users. So I personally
> > like to omit the dbname if "dbname=replication", at this point.
> >
>
> How about if we write dbname in primary_conninfo to
> postgresql.auto.conf file only when the user has explicitly specified
> dbname in the connection string? To achieve this we need to somehow
> pass this information via PGconn (say by having a new bool variable
> dbname_specified) from GetConnection() or something like that?
Here is a patch which will write dbname in the primary_conninfo only
if the database name is specified explicitly. I have added a new
function GetDbnameFromConnectionString which will return the dbname
specified in the connection and GenerateRecoveryConfig will append
this database name.
Here are the test results with the patch:
case 1:
pg_basebackup -D test10 -p 5431 -X s -P -R -d "user=vignesh dbname=postgres"
primary_conninfo will have dbname=postgres
case 2:
pg_basebackup -D test10 -p 5431 -X s -P -R -d "user=vignesh dbname=replication"
primary_conninfo will have dbname=replication
case 3:
pg_basebackup -D test10 -p 5431 -X s -P -R -U vignesh
primary_conninfo will not have dbname
case 4:
pg_basebackup -D test10 -p 5431 -X s -P -R
primary_conninfo will not have dbname
case 5:
pg_basebackup -D test10 -p 5431 -X s -P -R -d "user=vignesh"
primary_conninfo will not have dbname
case 6:
pg_basebackup -D test10 -p 5431 -X s -P -R -d ""
primary_conninfo will not have dbname
Thoughts?
Regards,
Vignesh
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 88c689e725..903da73df7 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.
+ streaming replication or <link linkend="logicaldecoding-replication-slots-synchronization">
+ logical replication slot synchronization</link> will use the same
+ settings later on. The dbname will be recorded only if the dbname was
+ specified explicitly in the connection string.
</para>
</listitem>
@@ -807,7 +810,9 @@ PostgreSQL documentation
client applications, but because <application>pg_basebackup</application>
doesn't connect to any particular database in the cluster, any database
name in the connection string will be ignored
- by <productname>PostgreSQL</productname>. Middleware, or proxies, used in
+ by <productname>PostgreSQL</productname>. Middleware, proxies, or
+ <link linkend="logicaldecoding-replication-slots-synchronization">
+ logical replication slot synchronization</link> used in
connecting to <productname>PostgreSQL</productname> might however
utilize the value.
</para>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 3a9940097c..8880e0bef6 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -1810,7 +1810,8 @@ BaseBackup(char *compression_algorithm, char *compression_detail,
* Build contents of configuration file if requested
*/
if (writerecoveryconf)
- recoveryconfcontents = GenerateRecoveryConfig(conn, replication_slot);
+ recoveryconfcontents = GenerateRecoveryConfig(conn, replication_slot,
+ GetDbnameFromConnectionString());
/*
* 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 56d1b15951..a88b660c8a 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -267,6 +267,38 @@ GetConnection(void)
return tmpconn;
}
+/*
+ * GetDbnameFromConnectionString
+ *
+ * If database is specified in the connection string, then return the last
+ * database name specified. If database is not specified in the connection
+ * string, then return NULL.
+ */
+char *
+GetDbnameFromConnectionString(void)
+{
+ PQconninfoOption *conn_opts = NULL;
+ PQconninfoOption *conn_opt;
+ char *err_msg = NULL;
+ char *dbname = NULL;
+
+ if (connection_string)
+ {
+ conn_opts = PQconninfoParse(connection_string, &err_msg);
+ if (conn_opts == NULL)
+ pg_fatal("%s", err_msg);
+
+ 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')
+ dbname = conn_opt->val;
+ }
+ }
+
+ 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 7a3dd98da3..1ade59247a 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -31,6 +31,8 @@ extern PGconn *conn;
extern PGconn *GetConnection(void);
+extern char *GetDbnameFromConnectionString(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 bde90bf60b..8449ae78ef 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -451,7 +451,7 @@ main(int argc, char **argv)
pg_log_info("no rewind required");
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL));
+ GenerateRecoveryConfig(conn, NULL, NULL));
exit(0);
}
@@ -525,7 +525,7 @@ main(int argc, char **argv)
/* Also update the standby configuration, if requested. */
if (writerecoveryconf && !dry_run)
WriteRecoveryConfig(conn, datadir_target,
- GenerateRecoveryConfig(conn, NULL));
+ GenerateRecoveryConfig(conn, NULL, NULL));
/* 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 2585f11939..0b5d9abc48 100644
--- a/src/fe_utils/recovery_gen.c
+++ b/src/fe_utils/recovery_gen.c
@@ -20,7 +20,7 @@ static char *escape_quotes(const char *src);
* return it.
*/
PQExpBuffer
-GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot)
+GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot, char *dbname)
{
PQconninfoOption *connOptions;
PQExpBufferData conninfo_buf;
@@ -66,6 +66,20 @@ GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot)
appendPQExpBuffer(&conninfo_buf, "%s=", opt->keyword);
appendConnStrVal(&conninfo_buf, opt->val);
}
+
+ if (dbname)
+ {
+ /*
+ * If dbname is specified in the connection, append the dbname. This
+ * will be used later for logical replication slot synchronization.
+ */
+ if (conninfo_buf.len != 0)
+ appendPQExpBufferChar(&conninfo_buf, ' ');
+
+ appendPQExpBuffer(&conninfo_buf, "%s=", "dbname");
+ appendConnStrVal(&conninfo_buf, dbname);
+ }
+
if (PQExpBufferDataBroken(conninfo_buf))
pg_fatal("out of memory");
diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h
index ca2c4800d0..5a986297a5 100644
--- a/src/include/fe_utils/recovery_gen.h
+++ b/src/include/fe_utils/recovery_gen.h
@@ -21,7 +21,8 @@
#define MINIMUM_VERSION_FOR_RECOVERY_GUC 120000
extern PQExpBuffer GenerateRecoveryConfig(PGconn *pgconn,
- char *replication_slot);
+ char *replication_slot,
+ char *dbname);
extern void WriteRecoveryConfig(PGconn *pgconn, char *target_dir,
PQExpBuffer contents);
