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

On Thu, Mar 14, 2024 at 1:45 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Mar 14, 2024 at 2:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Mar 14, 2024 at 5:57 AM Masahiko Sawada <sawada.mshk@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?

--
With Regards,
Amit Kapila.