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

On Thu, Mar 14, 2024 at 11:46 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 14 Mar 2024 at 15:49, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > 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?
>
> 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?

Thank you for updating the patch!

This behavior makes sense to me. But do we want to handle the case of
using environment variables too? IIUC,

pg_basebackup -D tmp -d "user=masahiko dbname=test_db"

is equivalent to:

PGDATABASE="user=masahiko dbname=test_db" pg_basebackup -D tmp

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com