Thread: pg_recvlogical requires -d but not described on the documentation
pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
Attachment
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
Attachment
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
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
Re: pg_recvlogical requires -d but not described on the documentation
From
"David G. Johnston"
Date:
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.
Re: pg_recvlogical requires -d but not described on the documentation
From
"David G. Johnston"
Date:
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.
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 apolicy that would alter the synopsis in the proposed manner. I’d suggest leaving it alone for now and tweak any of theoption 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
Attachment
Re: pg_recvlogical requires -d but not described on the documentation
From
"David G. Johnston"
Date:
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.
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
Attachment
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
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
Attachment
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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
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
RE: pg_recvlogical requires -d but not described on the documentation
From
"Hayato Kuroda (Fujitsu)"
Date:
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