On Wed, 13 Mar 2024 at 16:58, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Mar 12, 2024 at 5:13 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > On Mon, 11 Mar 2024 at 17:16, Amit Kapila <amit.kapil...@gmail.com> wrote:
> > >
> > > On Tue, Feb 27, 2024 at 2:07 PM Hayato Kuroda (Fujitsu)
> > > <kuroda.hay...@fujitsu.com> wrote:
> > > >
> > > > > 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.
> > > >
> > > > 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.
> > > >
> > >
> > > 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.
> >
>
> 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?
>
> 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.
>
> 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].
Thanks for the comments, the attached v4 version patch has the changes
for the same.
Regards,
Vignesh
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 88c689e725..563346dd71 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -243,7 +243,9 @@ 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 slots synchronization</link> 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'))
