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

On Tue, Mar 12, 2024 at 5:13 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Mon, 11 Mar 2024 at 17:16, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Feb 27, 2024 at 2:07 PM Hayato Kuroda (Fujitsu)
> > <kuroda(dot)hayato(at)fujitsu(dot)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].

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