Horton

Thread

Commits

  1. Allow pg_recvlogical --drop-slot to work without --dbname.

  2. doc: Clarify required options for each action in pg_recvlogical.

  3. Refactor replication connection code of various pg_basebackup utilities.

  1. pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-02-21T04:25:43Z

    Dear hackers,

Hi, I found the issue $SUBJECT. According to the doc [1]:

```
-d dbname
--dbname=dbname
The database to connect to. See the description of the actions for what this means in detail.
The dbname can be a connection string. If so, connection string parameters will override any
conflicting command line options. Defaults to the user name.
```

IIUC final statement implies --dbname can be omitted. If it is mandatory, it should be described
in the synopsis part - not written now. However, the command would fail when -d is missed.

```
$ pg_recvlogical -U postgres --start -S test -f -
pg_recvlogical: error: no database specified
pg_recvlogical: hint: Try "pg_recvlogical --help" for more information.
```

ISTM the inconsistency is introduced since the initial commit. I think they should be unified either
1) update the doc or 2) accept when -d is not specified. Personally, I like 2nd approach, pg_recvlogical
can follow the normal connection rule. I.e., 

a) Use PGDATABASE if specified
b) Check 'U' and PGUSER and use it if specified
c) Otherwise use the current OS user name

Attached patch tries to implement the 2nd approach. How do you think?

[1]: https://www.postgresql.org/docs/devel/app-pgrecvlogical.html

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  2. Re: pg_recvlogical requires -d but not described on the documentation

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-21T06:11:26Z

    On Fri, Feb 21, 2025 at 9:56 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear hackers,
>
> Hi, I found the issue $SUBJECT. According to the doc [1]:
>
> ```
> -d dbname
> --dbname=dbname
> The database to connect to. See the description of the actions for what this means in detail.
> The dbname can be a connection string. If so, connection string parameters will override any
> conflicting command line options. Defaults to the user name.
> ```
>
> IIUC final statement implies --dbname can be omitted. If it is mandatory, it should be described
> in the synopsis part - not written now. However, the command would fail when -d is missed.
>
> ```
> $ pg_recvlogical -U postgres --start -S test -f -
> pg_recvlogical: error: no database specified
> pg_recvlogical: hint: Try "pg_recvlogical --help" for more information.
> ```
>
> ISTM the inconsistency is introduced since the initial commit. I think they should be unified either
> 1) update the doc or 2) accept when -d is not specified. Personally, I like 2nd approach, pg_recvlogical
> can follow the normal connection rule. I.e.,
>

Given that the discrepancy has survived so long, it seems that users
always pass -d. And to some extent, requiring users to specify a
database instead of defaulting to one is safer practice. This avoids
users fetching changes from unexpected database/slot and cause further
database inconsistencies on the receiver side. I would just fix
documentation in this case.

> a) Use PGDATABASE if specified
> b) Check 'U' and PGUSER and use it if specified
> c) Otherwise use the current OS user name

If we want to go this route, it will be good to unify the treatment of
-d option at one place in code and reuse it everywhere. Thanks to your
investigations, we are seeing too many descripancies in -d option
being used in many utilities. Fixing all at once would be good. Also
it will be good to similarly unify documentation by writing -d
documentation at only place and reusing it everywhere. But I don't
know whether our documentation allows modularization and code-reuse.

--
Best Wishes,
Ashutosh Bapat

  3. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-02-21T08:55:11Z

    Dear Ashutosh,

Thanks for the reply.

> > ISTM the inconsistency is introduced since the initial commit. I think they should
> be unified either
> > 1) update the doc or 2) accept when -d is not specified. Personally, I like 2nd
> approach, pg_recvlogical
> > can follow the normal connection rule. I.e.,
> >
> 
> Given that the discrepancy has survived so long, it seems that users
> always pass -d. And to some extent, requiring users to specify a
> database instead of defaulting to one is safer practice. This avoids
> users fetching changes from unexpected database/slot and cause further
> database inconsistencies on the receiver side. I would just fix
> documentation in this case.

Something like attached, right? The fact that everyone has been set -d option
may be strong.

> > a) Use PGDATABASE if specified
> > b) Check 'U' and PGUSER and use it if specified
> > c) Otherwise use the current OS user name
> 
> If we want to go this route, it will be good to unify the treatment of
> -d option at one place in code and reuse it everywhere. Thanks to your
> investigations, we are seeing too many descripancies in -d option
> being used in many utilities. Fixing all at once would be good. Also
> it will be good to similarly unify documentation by writing -d
> documentation at only place and reusing it everywhere. But I don't
> know whether our documentation allows modularization and code-reuse.

Hmm, unify the treatment seems clean, but it may break current connection rules
for some application. I'm not sure now this is helpful for users.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  4. Re: pg_recvlogical requires -d but not described on the documentation

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-02-21T10:52:54Z

    On Fri, Feb 21, 2025 at 2:25 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Ashutosh,
>
> Thanks for the reply.
>
> > > ISTM the inconsistency is introduced since the initial commit. I think they should
> > be unified either
> > > 1) update the doc or 2) accept when -d is not specified. Personally, I like 2nd
> > approach, pg_recvlogical
> > > can follow the normal connection rule. I.e.,
> > >
> >
> > Given that the discrepancy has survived so long, it seems that users
> > always pass -d. And to some extent, requiring users to specify a
> > database instead of defaulting to one is safer practice. This avoids
> > users fetching changes from unexpected database/slot and cause further
> > database inconsistencies on the receiver side. I would just fix
> > documentation in this case.
>
> Something like attached, right? The fact that everyone has been set -d option
> may be strong.

This looks good to me. It builds with meson. Didn't check make.

>
> > > a) Use PGDATABASE if specified
> > > b) Check 'U' and PGUSER and use it if specified
> > > c) Otherwise use the current OS user name
> >
> > If we want to go this route, it will be good to unify the treatment of
> > -d option at one place in code and reuse it everywhere. Thanks to your
> > investigations, we are seeing too many descripancies in -d option
> > being used in many utilities. Fixing all at once would be good. Also
> > it will be good to similarly unify documentation by writing -d
> > documentation at only place and reusing it everywhere. But I don't
> > know whether our documentation allows modularization and code-reuse.
>
> Hmm, unify the treatment seems clean, but it may break current connection rules
> for some application. I'm not sure now this is helpful for users.

Hmm. I agree.

-- 
Best Wishes,
Ashutosh Bapat

  5. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-02-25T02:53:12Z

    Dear Ashutosh,

Thanks for the confirmation!

> This looks good to me. It builds with meson. Didn't check make.

I confirmed it could be built via make command.

> > Hmm, unify the treatment seems clean, but it may break current connection
> rules
> > for some application. I'm not sure now this is helpful for users.
> 
> Hmm. I agree.

OK, so I will exclude the point in the thread. The patch I've posted contains all of fixes
which is required.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  6. Re: pg_recvlogical requires -d but not described on the documentation

    vignesh C <vignesh21@gmail.com> — 2025-03-12T05:32:32Z

    On Fri, 21 Feb 2025 at 14:25, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Ashutosh,
>
> Thanks for the reply.
>
> > > ISTM the inconsistency is introduced since the initial commit. I think they should
> > be unified either
> > > 1) update the doc or 2) accept when -d is not specified. Personally, I like 2nd
> > approach, pg_recvlogical
> > > can follow the normal connection rule. I.e.,
> > >
> >
> > Given that the discrepancy has survived so long, it seems that users
> > always pass -d. And to some extent, requiring users to specify a
> > database instead of defaulting to one is safer practice. This avoids
> > users fetching changes from unexpected database/slot and cause further
> > database inconsistencies on the receiver side. I would just fix
> > documentation in this case.
>
> Something like attached, right?

Apart from the database, I believe the target file also needs to be
specified. Should we include this option also along with dbname:
+   <group choice="plain">
+    <group choice="req">
+     <arg choice="plain"><option>-d</option></arg>
+     <arg choice="plain"><option>--dbname</option></arg>
+    </group>
+    <replaceable>dbname</replaceable>
+   </group>

pg_recvlogical -U postgres --start -S test -d postgres
pg_recvlogical: error: no target file specified

Regards,
Vignesh

  7. Re: pg_recvlogical requires -d but not described on the documentation

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-12T05:59:09Z

    On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) <
kuroda.hayato@fujitsu.com> wrote:
>
>
> OK, so I will exclude the point in the thread. The patch I've posted
> contains all of fixes
> which is required.
>

The patch fixes the synopsis and the mention of the default value.  Only
the later is required.  I cannot figure out a policy that would alter the
synopsis in the proposed manner.  I’d suggest leaving it alone for now and
tweak any of the option descriptions that may need clarification.  At least
then we remain consistent with a policy that options are never listed in
client application synopses.  Which seems to be the policy that is in
force.  There is discussion to better document, and possibly change these
policies, especially since server application synopses routinely violate
this no-options policy.

David J.

  8. Re: pg_recvlogical requires -d but not described on the documentation

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-12T06:08:34Z

    On Tuesday, March 11, 2025, vignesh C <vignesh21@gmail.com> wrote:
>
>
> Apart from the database, I believe the target file also needs to be
> specified. Should we include this option also along with dbname:
> +   <group choice="plain">
> +    <group choice="req">
> +     <arg choice="plain"><option>-d</option></arg>
> +     <arg choice="plain"><option>--dbname</option></arg>
> +    </group>
> +    <replaceable>dbname</replaceable>
> +   </group>
>
> pg_recvlogical -U postgres --start -S test -d postgres
> pg_recvlogical: error: no target file specified
>

To reinforce my suggestion to “do nothing” here with the synopsis; your
“show mandatory” policy would extend to “—slot” as well as communicating
that ((create OR start) XOR drop) must also be specified.

I am fine with this “show mandatory” policy in concept but doing it just
here in lieu of an acceptable “no options” policy presently doesn’t make
sense.  There is debate, from me at least, whether such a command synopsis
should include both short and long options in an alternate group or just
short options, unless the option is a switch.  You went with both here
which I dislike.

David J.

  9. Re: pg_recvlogical requires -d but not described on the documentation

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-18T05:34:52Z

    
On 2025/03/12 14:59, David G. Johnston wrote:
> On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com <mailto:kuroda.hayato@fujitsu.com>> wrote:
> 
> 
>     OK, so I will exclude the point in the thread. The patch I've posted contains all of fixes
>     which is required.
> 
> 
> The patch fixes the synopsis and the mention of the default value.  Only the later is required.  I cannot figure out a policy that would alter the synopsis in the proposed manner.  I’d suggest leaving it alone for now and tweak any of the option descriptions that may need clarification.

I agree that the synopsis doesn't need to be updated. Attached patch clarifies
the required options for each action in the documentation. Thought?

BTW, I'm curious why --dbname isn't required for the --drop-slot action. When I ran
pg_recvlogical --drop-slot without --dbname, I got the following error:

   pg_recvlogical: error: could not establish database-specific replication connection

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  10. Re: pg_recvlogical requires -d but not described on the documentation

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-18T06:10:52Z

    On Monday, March 17, 2025, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

>
> On 2025/03/12 14:59, David G. Johnston wrote:
>
>> On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) <
>> kuroda.hayato@fujitsu.com <mailto:kuroda.hayato@fujitsu.com>> wrote:
>>
>>
>>     OK, so I will exclude the point in the thread. The patch I've posted
>> contains all of fixes
>>     which is required.
>>
>>
>> The patch fixes the synopsis and the mention of the default value.  Only
>> the later is required.  I cannot figure out a policy that would alter the
>> synopsis in the proposed manner.  I’d suggest leaving it alone for now and
>> tweak any of the option descriptions that may need clarification.
>>
>
> I agree that the synopsis doesn't need to be updated. Attached patch
> clarifies
> the required options for each action in the documentation. Thought?


Will look again tomorrow but seems ok aside from needing an editing pass.

The usage section for help probably needs a look as well.


>
> BTW, I'm curious why --dbname isn't required for the --drop-slot action.
> When I ran
> pg_recvlogical --drop-slot without --dbname, I got the following error:
>
>   pg_recvlogical: error: could not establish database-specific replication
> connection
>

That would be a bug.

I think this is too:

set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));

Though I’m not sure what it is doing.

David J.

  11. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-18T07:32:49Z

    Dear Fujii-san,

Thanks for giving comments and sorry for missing replies.

> I agree that the synopsis doesn't need to be updated. Attached patch clarifies
> the required options for each action in the documentation. Thought?

So, the policy David said is not to modify the synopsis part here, because there
are no rules or it is broken. Instead, we could clarify the mandatory options in
the doc, right?
I have no objections for it.

> BTW, I'm curious why --dbname isn't required for the --drop-slot action.

I'm analyzing around here...

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  12. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-18T07:33:15Z

    Dear David, Fujii-san,

> I think this is too:
> set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("pg_basebackup"));

I think it is intentional. IIUC, it accepts the directory which the file exists.
Please see commits fc995bfdbf.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  13. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-18T09:17:21Z

    Dear Fujii-san, David,

> > BTW, I'm curious why --dbname isn't required for the --drop-slot action.
> 
> I'm analyzing around here...
>

Actually, replication slots can be dropped from another database where it created,
or even from the streaming replication connection.
I forked the new thread which fixes the description [1].

Based on the fact, there are two approaches to fix:

1. Fix not to raise fatal error like:

```
@@ -950,7 +950,7 @@ main(int argc, char **argv)
        if (!RunIdentifySystem(conn, NULL, NULL, NULL, &db_name))
                exit(1);
 
-       if (db_name == NULL)
+       if (!do_drop_slot && db_name == NULL)
                pg_fatal("could not establish database-specific replication connection");
```

db_name == NULL means that streaming replication connection has been established,
so other operations are not allowed.

2. Fix documentation

We keep the current restriction and clarify it. For the reportability, it is
OK for me to also modify the code like:

```
@@ -881,7 +881,7 @@ main(int argc, char **argv)
                exit(1);
        }
 
-       if (!do_drop_slot && dbname == NULL)
+       if (dbname == NULL)
```

Thought?

[1]: https://www.postgresql.org/message-id/OSCPR01MB14966C6BE304B5BB2E58D4009F5DE2%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  14. Re: pg_recvlogical requires -d but not described on the documentation

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-18T16:37:19Z

    
On 2025/03/18 18:17, Hayato Kuroda (Fujitsu) wrote:
> Dear Fujii-san, David,
> 
>>> BTW, I'm curious why --dbname isn't required for the --drop-slot action.
>>
>> I'm analyzing around here...
>>
> 
> Actually, replication slots can be dropped from another database where it created,
> or even from the streaming replication connection.
> I forked the new thread which fixes the description [1].
> 
> Based on the fact, there are two approaches to fix:
> 
> 1. Fix not to raise fatal error like:


It looks like commit 0c013e08cfb introduced a bug that causes "pg_recvlogical --drop-slot"
without --dbname to check whether it's connected to a specific database and fail if it's not.

This commit was added before 9.5, while pg_recvlogical was introduced in 9.4. On my env,
"pg_recvlogical --drop-slot" without --dbname worked as expected in 9.4 but started
failing in 9.5 or later.

So, I think the proper fix is to avoid raising a fatal error even when not connected to
a specific database in --drop-slot action.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  15. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-19T02:32:26Z

    Dear Fujii-san,

> It looks like commit 0c013e08cfb introduced a bug that causes "pg_recvlogical
> --drop-slot"
> without --dbname to check whether it's connected to a specific database and fail
> if it's not.
> 
> This commit was added before 9.5, while pg_recvlogical was introduced in 9.4. On
> my env,
> "pg_recvlogical --drop-slot" without --dbname worked as expected in 9.4 but
> started
> failing in 9.5 or later.
> 
> So, I think the proper fix is to avoid raising a fatal error even when not connected
> to
> a specific database in --drop-slot action.

+1. I created patch to fix it. 0001 was completely same as you did.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  16. Re: pg_recvlogical requires -d but not described on the documentation

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-19T17:51:43Z

    
On 2025/03/19 11:32, Hayato Kuroda (Fujitsu) wrote:
> Dear Fujii-san,
> 
>> It looks like commit 0c013e08cfb introduced a bug that causes "pg_recvlogical
>> --drop-slot"
>> without --dbname to check whether it's connected to a specific database and fail
>> if it's not.
>>
>> This commit was added before 9.5, while pg_recvlogical was introduced in 9.4. On
>> my env,
>> "pg_recvlogical --drop-slot" without --dbname worked as expected in 9.4 but
>> started
>> failing in 9.5 or later.
>>
>> So, I think the proper fix is to avoid raising a fatal error even when not connected
>> to
>> a specific database in --drop-slot action.
> 
> +1. I created patch to fix it. 0001 was completely same as you did.

Thanks for the patch! It looks good to me.

I'm considering whether to back-patch these changes to older versions.
Since pg_recvlogical --drop-slot worked without --dbname in 9.4
but started failing unintentionally in 9.5, it could be considered a bug.
However, this behavior has existed for a long time without complaints or
bug reports, and there was no clear documentation stating that
--drop-slot should work without --dbname.

Given this, I think that also we could treat it as not a bug and apply
the change only to the master branch. What do you think should we
back-patch it as a bug fix or apply it only to master?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  17. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-21T01:12:26Z

    Dear Fujii-san,

> Thanks for the patch! It looks good to me.
> 
> I'm considering whether to back-patch these changes to older versions.
> Since pg_recvlogical --drop-slot worked without --dbname in 9.4
> but started failing unintentionally in 9.5, it could be considered a bug.
> However, this behavior has existed for a long time without complaints or
> bug reports, and there was no clear documentation stating that
> --drop-slot should work without --dbname.
> 
> Given this, I think that also we could treat it as not a bug and apply
> the change only to the master branch. What do you think should we
> back-patch it as a bug fix or apply it only to master?

Personally considered, such a long-standing but harmless bug can be regarded as
the specification. So, I vote that this is an enhancement and be applied only to
master.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  18. Re: pg_recvlogical requires -d but not described on the documentation

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-23T17:13:42Z

    
On 2025/03/21 10:12, Hayato Kuroda (Fujitsu) wrote:
> Dear Fujii-san,
> 
>> Thanks for the patch! It looks good to me.
>>
>> I'm considering whether to back-patch these changes to older versions.
>> Since pg_recvlogical --drop-slot worked without --dbname in 9.4
>> but started failing unintentionally in 9.5, it could be considered a bug.
>> However, this behavior has existed for a long time without complaints or
>> bug reports, and there was no clear documentation stating that
>> --drop-slot should work without --dbname.
>>
>> Given this, I think that also we could treat it as not a bug and apply
>> the change only to the master branch. What do you think should we
>> back-patch it as a bug fix or apply it only to master?
> 
> Personally considered, such a long-standing but harmless bug can be regarded as
> the specification. So, I vote that this is an enhancement and be applied only to
> master.

+1

I've updated the commit messages for both patches and also revised
the code comments in the 0002 patch. The updated patches are attached.

Unless there are any objections, I'm thinking to commit them.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  19. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-24T02:21:43Z

    Dear Fujii-san,

> I've updated the commit messages for both patches and also revised
> the code comments in the 0002 patch. The updated patches are attached.
> 
> Unless there are any objections, I'm thinking to commit them.

Thanks for updating the patch. LGTM.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  20. Re: pg_recvlogical requires -d but not described on the documentation

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2025-03-24T15:56:34Z

    
On 2025/03/24 11:21, Hayato Kuroda (Fujitsu) wrote:
> Dear Fujii-san,
> 
>> I've updated the commit messages for both patches and also revised
>> the code comments in the 0002 patch. The updated patches are attached.
>>
>> Unless there are any objections, I'm thinking to commit them.
> 
> Thanks for updating the patch. LGTM.

I've pushed the patches. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  21. RE: pg_recvlogical requires -d but not described on the documentation

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-03-25T02:10:55Z

    Dear hackers,

> I've pushed the patches. Thanks!

This is a closing post. Fujii-san has pushed patches and no BF failures till now.
This patch does not modify the synopsis part, but it is intentional.
Per [1], we would discuss the manner for documentations in another thread.

I've closed the CF entry as "committed". Thanks!

[1]: https://www.postgresql.org/message-id/CAKFQuwYWVT84GM2OqRx8EqNrfzNM-zbpQ5Y2bA1dPO9jUgo_Kg%40mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED