public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/1] Fix printing last progress report line in client programs. 20+ messages / 6 participants [nested] [flat]
* [PATCH 1/1] Fix printing last progress report line in client programs. @ 2020-08-12 20:27 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Heikki Linnakangas @ 2020-08-12 20:27 UTC (permalink / raw) A number of client programs have a "--progress" option that when printing to a TTY, updates the current line by printing a '\r' and overwriting it. After the last line, an extra '\n' needs to be printed to move the cursor to the next line. pg_basebackup and pgbench got this right, but pg_rewind and pg_checksums were slightly wrong. pg_rewind printed the newline to stdout instead of stderr, and pg_checksum printed the newline even when not printing to a TTY. Fix them. --- src/bin/pg_checksums/pg_checksums.c | 3 ++- src/bin/pg_rewind/pg_rewind.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c index 1daa5aed0e0..9ae884897ef 100644 --- a/src/bin/pg_checksums/pg_checksums.c +++ b/src/bin/pg_checksums/pg_checksums.c @@ -626,7 +626,8 @@ main(int argc, char *argv[]) if (showprogress) { progress_report(true); - fprintf(stderr, "\n"); /* Need to move to next line */ + if (isatty(fileno(stderr))) + fprintf(stderr, "\n"); /* Need to move to next line */ } printf(_("Checksum operation completed\n")); diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 0015d3b461a..c000c12fa58 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -422,7 +422,8 @@ main(int argc, char **argv) executeFileMap(); progress_report(true); - printf("\n"); + if (showprogress && isatty(fileno(stderr))) + fprintf(stderr, "\n"); /* Need to move to next line */ if (showprogress) pg_log_info("creating backup label and updating control file"); -- 2.20.1 --------------D7F229EC53544D857CE3A506-- ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-02-21 08:55 Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 2 replies; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-02-21 08:55 UTC (permalink / raw) To: 'Ashutosh Bapat' <[email protected]>; +Cc: [email protected] <[email protected]> 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 Attachments: [application/octet-stream] 0001-pg_recvlogical-Make-sure-d-is-mandatory-option.patch (1.4K, ../../OSCPR01MB149660D1F436F8B82912FCB53F5C72@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-0001-pg_recvlogical-Make-sure-d-is-mandatory-option.patch) download | inline diff: From 342690e8af038578bbedf834caae85bcdbc2a5e0 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Fri, 21 Feb 2025 17:04:17 +0900 Subject: [PATCH] pg_recvlogical: Make sure -d is mandatory option --- doc/src/sgml/ref/pg_recvlogical.sgml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b635..5e59611de7 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -23,6 +23,13 @@ PostgreSQL documentation <cmdsynopsis> <command>pg_recvlogical</command> <arg rep="repeat" choice="opt"><replaceable>option</replaceable></arg> + <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> </cmdsynopsis> </refsynopsisdiv> @@ -305,7 +312,7 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. </para> </listitem> </varlistentry> -- 2.43.5 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-02-21 10:52 Ashutosh Bapat <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: Ashutosh Bapat @ 2025-02-21 10:52 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: [email protected] <[email protected]> On Fri, Feb 21, 2025 at 2:25 PM Hayato Kuroda (Fujitsu) <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-02-25 02:53 Hayato Kuroda (Fujitsu) <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-02-25 02:53 UTC (permalink / raw) To: 'Ashutosh Bapat' <[email protected]>; +Cc: [email protected] <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-12 05:32 vignesh C <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: vignesh C @ 2025-03-12 05:32 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> On Fri, 21 Feb 2025 at 14:25, Hayato Kuroda (Fujitsu) <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-12 05:59 David G. Johnston <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: David G. Johnston @ 2025-03-12 05:59 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) < [email protected]> 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. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-12 06:08 David G. Johnston <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: David G. Johnston @ 2025-03-12 06:08 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> On Tuesday, March 11, 2025, vignesh C <[email protected]> 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. ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 05:34 Fujii Masao <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 2 replies; 20+ messages in thread From: Fujii Masao @ 2025-03-18 05:34 UTC (permalink / raw) To: David G. Johnston <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> On 2025/03/12 14:59, David G. Johnston wrote: > On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) <[email protected] <mailto:[email protected]>> 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 From 8dcc53a6bebeaa1c4d0ddaacd35a3c450327bfef Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 18 Mar 2025 10:07:08 +0900 Subject: [PATCH v2] doc: Clarify required options for each action in pg_recvlogical Each pg_recvlogical action requires specific options. For example, --slot, --dbname, and --file must be specified with the --start action. Previously, the documentation did not clearly outline these requirements. This commit updates the documentation to explicitly state the necessary options for each action. --- doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b6352..2946bdae1e5 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -73,6 +73,11 @@ PostgreSQL documentation by <option>--dbname</option>. </para> + <para> + The <option>--slot</option> and <option>--dbname</option> are required + for this action. + </para> + <para> The <option>--two-phase</option> can be specified with <option>--create-slot</option> to enable decoding of prepared transactions. @@ -87,6 +92,10 @@ PostgreSQL documentation Drop the replication slot with the name specified by <option>--slot</option>, then exit. </para> + + <para> + The <option>--slot</option> is required for this action. + </para> </listitem> </varlistentry> @@ -101,6 +110,11 @@ PostgreSQL documentation <option>--no-loop</option> is specified. </para> + <para> + The <option>--slot</option> and <option>--dbname</option>, + <option>--file</option> are required for this action. + </para> + <para> The stream format is determined by the output plugin specified when the slot was created. @@ -159,6 +173,9 @@ PostgreSQL documentation Write received and decoded transaction data into this file. Use <literal>-</literal> for <systemitem>stdout</systemitem>. </para> + <para> + This parameter is required for <option>--start</option>. + </para> </listitem> </varlistentry> @@ -265,6 +282,9 @@ PostgreSQL documentation mode, create the slot with this name. In <option>--drop-slot</option> mode, delete the slot with this name. </para> + <para> + This parameter is required for any of actions. + </para> </listitem> </varlistentry> @@ -305,7 +325,11 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. + </para> + <para> + This parameter is required for <option>--create-slot</option> + and <option>--start</option>. </para> </listitem> </varlistentry> -- 2.48.1 Attachments: [text/plain] v2-0001-doc-Clarify-required-options-for-each-action-in-p.patch (3.1K, ../../[email protected]/2-v2-0001-doc-Clarify-required-options-for-each-action-in-p.patch) download | inline diff: From 8dcc53a6bebeaa1c4d0ddaacd35a3c450327bfef Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 18 Mar 2025 10:07:08 +0900 Subject: [PATCH v2] doc: Clarify required options for each action in pg_recvlogical Each pg_recvlogical action requires specific options. For example, --slot, --dbname, and --file must be specified with the --start action. Previously, the documentation did not clearly outline these requirements. This commit updates the documentation to explicitly state the necessary options for each action. --- doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b6352..2946bdae1e5 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -73,6 +73,11 @@ PostgreSQL documentation by <option>--dbname</option>. </para> + <para> + The <option>--slot</option> and <option>--dbname</option> are required + for this action. + </para> + <para> The <option>--two-phase</option> can be specified with <option>--create-slot</option> to enable decoding of prepared transactions. @@ -87,6 +92,10 @@ PostgreSQL documentation Drop the replication slot with the name specified by <option>--slot</option>, then exit. </para> + + <para> + The <option>--slot</option> is required for this action. + </para> </listitem> </varlistentry> @@ -101,6 +110,11 @@ PostgreSQL documentation <option>--no-loop</option> is specified. </para> + <para> + The <option>--slot</option> and <option>--dbname</option>, + <option>--file</option> are required for this action. + </para> + <para> The stream format is determined by the output plugin specified when the slot was created. @@ -159,6 +173,9 @@ PostgreSQL documentation Write received and decoded transaction data into this file. Use <literal>-</literal> for <systemitem>stdout</systemitem>. </para> + <para> + This parameter is required for <option>--start</option>. + </para> </listitem> </varlistentry> @@ -265,6 +282,9 @@ PostgreSQL documentation mode, create the slot with this name. In <option>--drop-slot</option> mode, delete the slot with this name. </para> + <para> + This parameter is required for any of actions. + </para> </listitem> </varlistentry> @@ -305,7 +325,11 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. + </para> + <para> + This parameter is required for <option>--create-slot</option> + and <option>--start</option>. </para> </listitem> </varlistentry> -- 2.48.1 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 06:10 David G. Johnston <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: David G. Johnston @ 2025-03-18 06:10 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> On Monday, March 17, 2025, Fujii Masao <[email protected]> wrote: > > On 2025/03/12 14:59, David G. Johnston wrote: > >> On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) < >> [email protected] <mailto:[email protected]>> 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. ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 07:32 Hayato Kuroda (Fujitsu) <[email protected]> parent: Fujii Masao <[email protected]> 1 sibling, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-18 07:32 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; 'vignesh C' <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 07:33 Hayato Kuroda (Fujitsu) <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-18 07:33 UTC (permalink / raw) To: 'David G. Johnston' <[email protected]>; Fujii Masao <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 09:17 Hayato Kuroda (Fujitsu) <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-18 09:17 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; Hayato Kuroda (Fujitsu) <[email protected]>; 'vignesh C' <[email protected]> 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.jpnpr... Best regards, Hayato Kuroda FUJITSU LIMITED ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-18 16:37 Fujii Masao <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Fujii Masao @ 2025-03-18 16:37 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-19 02:32 Hayato Kuroda (Fujitsu) <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-19 02:32 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 Attachments: [application/octet-stream] v3-0001-doc-Clarify-required-options-for-each-action-in-p.patch (3.1K, ../../OSCPR01MB149660CCE2ADCC54A11F57E09F5D92@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v3-0001-doc-Clarify-required-options-for-each-action-in-p.patch) download | inline diff: From 959222ee6d2f83308324253bcbe3add3466b9533 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 18 Mar 2025 10:07:08 +0900 Subject: [PATCH v3 1/2] doc: Clarify required options for each action in pg_recvlogical Each pg_recvlogical action requires specific options. For example, --slot, --dbname, and --file must be specified with the --start action. Previously, the documentation did not clearly outline these requirements. This commit updates the documentation to explicitly state the necessary options for each action. --- doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b6352..2946bdae1e5 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -73,6 +73,11 @@ PostgreSQL documentation by <option>--dbname</option>. </para> + <para> + The <option>--slot</option> and <option>--dbname</option> are required + for this action. + </para> + <para> The <option>--two-phase</option> can be specified with <option>--create-slot</option> to enable decoding of prepared transactions. @@ -87,6 +92,10 @@ PostgreSQL documentation Drop the replication slot with the name specified by <option>--slot</option>, then exit. </para> + + <para> + The <option>--slot</option> is required for this action. + </para> </listitem> </varlistentry> @@ -101,6 +110,11 @@ PostgreSQL documentation <option>--no-loop</option> is specified. </para> + <para> + The <option>--slot</option> and <option>--dbname</option>, + <option>--file</option> are required for this action. + </para> + <para> The stream format is determined by the output plugin specified when the slot was created. @@ -159,6 +173,9 @@ PostgreSQL documentation Write received and decoded transaction data into this file. Use <literal>-</literal> for <systemitem>stdout</systemitem>. </para> + <para> + This parameter is required for <option>--start</option>. + </para> </listitem> </varlistentry> @@ -265,6 +282,9 @@ PostgreSQL documentation mode, create the slot with this name. In <option>--drop-slot</option> mode, delete the slot with this name. </para> + <para> + This parameter is required for any of actions. + </para> </listitem> </varlistentry> @@ -305,7 +325,11 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. + </para> + <para> + This parameter is required for <option>--create-slot</option> + and <option>--start</option>. </para> </listitem> </varlistentry> -- 2.43.5 [application/octet-stream] v3-0002-pg_recvlogical-allow-working-without-dbname-while.patch (1.6K, ../../OSCPR01MB149660CCE2ADCC54A11F57E09F5D92@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v3-0002-pg_recvlogical-allow-working-without-dbname-while.patch) download | inline diff: From 75e0b8649c7370132919e95870cd712bfc33020d Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Wed, 19 Mar 2025 11:30:16 +0900 Subject: [PATCH v3 2/2] pg_recvlogical: allow working without --dbname while in drop mode --- src/bin/pg_basebackup/pg_recvlogical.c | 3 ++- src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index b9ea23e1426..17c64146eff 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -945,12 +945,13 @@ main(int argc, char **argv) /* * Run IDENTIFY_SYSTEM to make sure we connected using a database specific + * replication connection. Drop-slot action can work with streaming * replication connection. */ 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"); /* diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl index a6e10600161..62bbc5a3f98 100644 --- a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl +++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl @@ -127,4 +127,12 @@ $node->command_ok( ], 'replayed a two-phase transaction'); +$node->command_ok( + [ + 'pg_recvlogical', + '--slot' => 'test', + '--drop-slot' + ], + 'drop could work without dbname'); + done_testing(); -- 2.43.5 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-19 17:51 Fujii Masao <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Fujii Masao @ 2025-03-19 17:51 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-21 01:12 Hayato Kuroda (Fujitsu) <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-21 01:12 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-23 17:13 Fujii Masao <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Fujii Masao @ 2025-03-23 17:13 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 From 53360ffb7a908a881a94ff81934dca9a7825d8d1 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 18 Mar 2025 10:07:08 +0900 Subject: [PATCH v4] doc: Clarify required options for each action in pg_recvlogical Each pg_recvlogical action requires specific options. For example, --slot, --dbname, and --file must be specified with the --start action. Previously, the documentation did not clearly outline these requirements. This commit updates the documentation to explicitly state the necessary options for each action. Author: Hayato Kuroda <[email protected]> Co-authored-by: Fujii Masao <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Reviewed-by: Vignesh C <[email protected]> Reviewed-by: David G. Johnston <[email protected]> Discussion: https://postgr.es/m/OSCPR01MB14966930B4357BAE8C9D68A8AF5C72@OSCPR01MB14966.jpnprd01.prod.outlook.com --- doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b6352..2946bdae1e5 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -73,6 +73,11 @@ PostgreSQL documentation by <option>--dbname</option>. </para> + <para> + The <option>--slot</option> and <option>--dbname</option> are required + for this action. + </para> + <para> The <option>--two-phase</option> can be specified with <option>--create-slot</option> to enable decoding of prepared transactions. @@ -87,6 +92,10 @@ PostgreSQL documentation Drop the replication slot with the name specified by <option>--slot</option>, then exit. </para> + + <para> + The <option>--slot</option> is required for this action. + </para> </listitem> </varlistentry> @@ -101,6 +110,11 @@ PostgreSQL documentation <option>--no-loop</option> is specified. </para> + <para> + The <option>--slot</option> and <option>--dbname</option>, + <option>--file</option> are required for this action. + </para> + <para> The stream format is determined by the output plugin specified when the slot was created. @@ -159,6 +173,9 @@ PostgreSQL documentation Write received and decoded transaction data into this file. Use <literal>-</literal> for <systemitem>stdout</systemitem>. </para> + <para> + This parameter is required for <option>--start</option>. + </para> </listitem> </varlistentry> @@ -265,6 +282,9 @@ PostgreSQL documentation mode, create the slot with this name. In <option>--drop-slot</option> mode, delete the slot with this name. </para> + <para> + This parameter is required for any of actions. + </para> </listitem> </varlistentry> @@ -305,7 +325,11 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. + </para> + <para> + This parameter is required for <option>--create-slot</option> + and <option>--start</option>. </para> </listitem> </varlistentry> -- 2.48.1 From 44c686ae11cc0de8d0aaf70c23397f94b477a82a Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Wed, 19 Mar 2025 11:30:16 +0900 Subject: [PATCH v4 2/2] Allow pg_recvlogical --drop-slot to work without --dbname. When pg_recvlogical was introduced in 9.4, the --dbname option was not required for --drop-slot. Without it, pg_recvlogical --drop-slot connected using a replication connection (not tied to a specific database) and was able to drop both physical and logical replication slots, similar to pg_receivewal --drop-slot. However, commit 0c013e08cfb unintentionally changed this behavior in 9.5, making pg_recvlogical always check whether it's connected to a specific database and fail if it's not. This change was expected for --create-slot and --start, which handle logical replication slots and require a database connection, but it was unnecessary for --drop-slot, which should work with any replication connection. As a result, --dbname became a required option for --drop-slot. This commit removes that restriction, restoring the original behavior and allowing pg_recvlogical --drop-slot to work without specifying --dbname. Although this issue originated from an unintended change, it has existed for a long time without complaints or bug reports, and the documentation never explicitly stated that --drop-slot should work without --dbname. Therefore, the change is not treated as a bug fix and is applied only to master. Author: Hayato Kuroda <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++++--- src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index b9ea23e1426..a3447753119 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -944,13 +944,16 @@ main(int argc, char **argv) #endif /* - * Run IDENTIFY_SYSTEM to make sure we connected using a database specific - * replication connection. + * Run IDENTIFY_SYSTEM to check the connection type for each action. + * --create-slot and --start actions require a database-specific + * replication connection because they handle logical replication slots. + * --drop-slot can remove replication slots from any replication + * connection without this restriction. */ 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"); /* diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl index a6e10600161..62bbc5a3f98 100644 --- a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl +++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl @@ -127,4 +127,12 @@ $node->command_ok( ], 'replayed a two-phase transaction'); +$node->command_ok( + [ + 'pg_recvlogical', + '--slot' => 'test', + '--drop-slot' + ], + 'drop could work without dbname'); + done_testing(); -- 2.48.1 Attachments: [text/plain] v4-0001-doc-Clarify-required-options-for-each-action-in-p.patch (3.5K, ../../[email protected]/2-v4-0001-doc-Clarify-required-options-for-each-action-in-p.patch) download | inline diff: From 53360ffb7a908a881a94ff81934dca9a7825d8d1 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Tue, 18 Mar 2025 10:07:08 +0900 Subject: [PATCH v4] doc: Clarify required options for each action in pg_recvlogical Each pg_recvlogical action requires specific options. For example, --slot, --dbname, and --file must be specified with the --start action. Previously, the documentation did not clearly outline these requirements. This commit updates the documentation to explicitly state the necessary options for each action. Author: Hayato Kuroda <[email protected]> Co-authored-by: Fujii Masao <[email protected]> Reviewed-by: Ashutosh Bapat <[email protected]> Reviewed-by: Vignesh C <[email protected]> Reviewed-by: David G. Johnston <[email protected]> Discussion: https://postgr.es/m/OSCPR01MB14966930B4357BAE8C9D68A8AF5C72@OSCPR01MB14966.jpnprd01.prod.outlook.com --- doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/doc/src/sgml/ref/pg_recvlogical.sgml b/doc/src/sgml/ref/pg_recvlogical.sgml index 95eb14b6352..2946bdae1e5 100644 --- a/doc/src/sgml/ref/pg_recvlogical.sgml +++ b/doc/src/sgml/ref/pg_recvlogical.sgml @@ -73,6 +73,11 @@ PostgreSQL documentation by <option>--dbname</option>. </para> + <para> + The <option>--slot</option> and <option>--dbname</option> are required + for this action. + </para> + <para> The <option>--two-phase</option> can be specified with <option>--create-slot</option> to enable decoding of prepared transactions. @@ -87,6 +92,10 @@ PostgreSQL documentation Drop the replication slot with the name specified by <option>--slot</option>, then exit. </para> + + <para> + The <option>--slot</option> is required for this action. + </para> </listitem> </varlistentry> @@ -101,6 +110,11 @@ PostgreSQL documentation <option>--no-loop</option> is specified. </para> + <para> + The <option>--slot</option> and <option>--dbname</option>, + <option>--file</option> are required for this action. + </para> + <para> The stream format is determined by the output plugin specified when the slot was created. @@ -159,6 +173,9 @@ PostgreSQL documentation Write received and decoded transaction data into this file. Use <literal>-</literal> for <systemitem>stdout</systemitem>. </para> + <para> + This parameter is required for <option>--start</option>. + </para> </listitem> </varlistentry> @@ -265,6 +282,9 @@ PostgreSQL documentation mode, create the slot with this name. In <option>--drop-slot</option> mode, delete the slot with this name. </para> + <para> + This parameter is required for any of actions. + </para> </listitem> </varlistentry> @@ -305,7 +325,11 @@ PostgreSQL documentation The <replaceable>dbname</replaceable> can be a <link linkend="libpq-connstring">connection string</link>. If so, connection string parameters will override any conflicting - command line options. Defaults to the user name. + command line options. + </para> + <para> + This parameter is required for <option>--create-slot</option> + and <option>--start</option>. </para> </listitem> </varlistentry> -- 2.48.1 [text/plain] v4-0002-Allow-pg_recvlogical-drop-slot-to-work-without-db.patch (3.2K, ../../[email protected]/3-v4-0002-Allow-pg_recvlogical-drop-slot-to-work-without-db.patch) download | inline diff: From 44c686ae11cc0de8d0aaf70c23397f94b477a82a Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Wed, 19 Mar 2025 11:30:16 +0900 Subject: [PATCH v4 2/2] Allow pg_recvlogical --drop-slot to work without --dbname. When pg_recvlogical was introduced in 9.4, the --dbname option was not required for --drop-slot. Without it, pg_recvlogical --drop-slot connected using a replication connection (not tied to a specific database) and was able to drop both physical and logical replication slots, similar to pg_receivewal --drop-slot. However, commit 0c013e08cfb unintentionally changed this behavior in 9.5, making pg_recvlogical always check whether it's connected to a specific database and fail if it's not. This change was expected for --create-slot and --start, which handle logical replication slots and require a database connection, but it was unnecessary for --drop-slot, which should work with any replication connection. As a result, --dbname became a required option for --drop-slot. This commit removes that restriction, restoring the original behavior and allowing pg_recvlogical --drop-slot to work without specifying --dbname. Although this issue originated from an unintended change, it has existed for a long time without complaints or bug reports, and the documentation never explicitly stated that --drop-slot should work without --dbname. Therefore, the change is not treated as a bug fix and is applied only to master. Author: Hayato Kuroda <[email protected]> Reviewed-by: Fujii Masao <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/bin/pg_basebackup/pg_recvlogical.c | 9 ++++++--- src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c index b9ea23e1426..a3447753119 100644 --- a/src/bin/pg_basebackup/pg_recvlogical.c +++ b/src/bin/pg_basebackup/pg_recvlogical.c @@ -944,13 +944,16 @@ main(int argc, char **argv) #endif /* - * Run IDENTIFY_SYSTEM to make sure we connected using a database specific - * replication connection. + * Run IDENTIFY_SYSTEM to check the connection type for each action. + * --create-slot and --start actions require a database-specific + * replication connection because they handle logical replication slots. + * --drop-slot can remove replication slots from any replication + * connection without this restriction. */ 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"); /* diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl index a6e10600161..62bbc5a3f98 100644 --- a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl +++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl @@ -127,4 +127,12 @@ $node->command_ok( ], 'replayed a two-phase transaction'); +$node->command_ok( + [ + 'pg_recvlogical', + '--slot' => 'test', + '--drop-slot' + ], + 'drop could work without dbname'); + done_testing(); -- 2.48.1 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-24 02:21 Hayato Kuroda (Fujitsu) <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-24 02:21 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* Re: pg_recvlogical requires -d but not described on the documentation @ 2025-03-24 15:56 Fujii Masao <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 20+ messages in thread From: Fujii Masao @ 2025-03-24 15:56 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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 ^ permalink raw reply [nested|flat] 20+ messages in thread
* RE: pg_recvlogical requires -d but not described on the documentation @ 2025-03-25 02:10 Hayato Kuroda (Fujitsu) <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 0 replies; 20+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2025-03-25 02:10 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; David G. Johnston <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; [email protected] <[email protected]>; 'vignesh C' <[email protected]> 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.gma... Best regards, Hayato Kuroda FUJITSU LIMITED ^ permalink raw reply [nested|flat] 20+ messages in thread
end of thread, other threads:[~2025-03-25 02:10 UTC | newest] Thread overview: 20+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-12 20:27 [PATCH 1/1] Fix printing last progress report line in client programs. Heikki Linnakangas <[email protected]> 2025-02-21 08:55 RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-02-21 10:52 ` Re: pg_recvlogical requires -d but not described on the documentation Ashutosh Bapat <[email protected]> 2025-02-25 02:53 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-12 05:59 ` Re: pg_recvlogical requires -d but not described on the documentation David G. Johnston <[email protected]> 2025-03-18 05:34 ` Re: pg_recvlogical requires -d but not described on the documentation Fujii Masao <[email protected]> 2025-03-18 06:10 ` Re: pg_recvlogical requires -d but not described on the documentation David G. Johnston <[email protected]> 2025-03-18 07:33 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-18 07:32 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-18 09:17 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-18 16:37 ` Re: pg_recvlogical requires -d but not described on the documentation Fujii Masao <[email protected]> 2025-03-19 02:32 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-19 17:51 ` Re: pg_recvlogical requires -d but not described on the documentation Fujii Masao <[email protected]> 2025-03-21 01:12 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-23 17:13 ` Re: pg_recvlogical requires -d but not described on the documentation Fujii Masao <[email protected]> 2025-03-24 02:21 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-24 15:56 ` Re: pg_recvlogical requires -d but not described on the documentation Fujii Masao <[email protected]> 2025-03-25 02:10 ` RE: pg_recvlogical requires -d but not described on the documentation Hayato Kuroda (Fujitsu) <[email protected]> 2025-03-12 05:32 ` Re: pg_recvlogical requires -d but not described on the documentation vignesh C <[email protected]> 2025-03-12 06:08 ` Re: pg_recvlogical requires -d but not described on the documentation David G. Johnston <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox