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

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 21 Feb 2025 08:55:11 +0000

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

---

From: Ashutosh Bapat <[email-masked]>
Date: Fri, 21 Feb 2025 16:22:54 +0530

> Something like attached, right? The fact that everyone has been set -d option

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

> for some application. I'm not sure now this is helpful for users.

Hmm. I agree.

-- 
Best Wishes,
Ashutosh Bapat

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 25 Feb 2025 02:53:12 +0000

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.

> > for some application. I'm not sure now this is helpful for users.

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

---

From: vignesh C <[email-masked]>
Date: Wed, 12 Mar 2025 11:02:32 +0530

On Fri, 21 Feb 2025 at 14:25, Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> 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

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 22:59:09 -0700

> OK, so I will exclude the point in the thread. The patch I've posted

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.

---

From: "David G. Johnston" <[email-masked]>
Date: Tue, 11 Mar 2025 23:08:34 -0700

> 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.

---

From: Fujii Masao <[email-masked]>
Date: Tue, 18 Mar 2025 14:34:52 +0900

On 2025/03/12 14:59, David G. Johnston wrote:
> On Monday, February 24, 2025, Hayato Kuroda (Fujitsu) <[email-masked] <mailto:[email-masked]>> wrote:
> 
> 
> 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-masked]>
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

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 17 Mar 2025 23:10:52 -0700

On Monday, March 17, 2025, Fujii Masao <[email-masked]> wrote:

>> [email-masked] <mailto:[email-masked]>> wrote:
>>
>>
>> tweak any of the option descriptions that may need clarification.
>>
>

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

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

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

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.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 18 Mar 2025 07:32:49 +0000

Dear Fujii-san,

Thanks for giving comments and sorry for missing replies.

> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 18 Mar 2025 07:33:15 +0000

Dear David, Fujii-san,

> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 18 Mar 2025 09:17:21 +0000

Dear Fujii-san, David,

> 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

---

From: Fujii Masao <[email-masked]>
Date: Wed, 19 Mar 2025 01:37:19 +0900

> 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,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 19 Mar 2025 02:32:26 +0000

Dear Fujii-san,

> 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

---

From: Fujii Masao <[email-masked]>
Date: Thu, 20 Mar 2025 02:51:43 +0900

> +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,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 21 Mar 2025 01:12:26 +0000

Dear Fujii-san,

> 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

---

From: Fujii Masao <[email-masked]>
Date: Mon, 24 Mar 2025 02:13:42 +0900

> the specification. So, I vote that this is an enhancement and be applied only to

+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-masked]>
Date: Tue, 18 Mar 2025 10:07:08 +0900
Subject: [PATCH v4] doc: Clarify required options for each action in
 pg_recvlogical

Author: Hayato Kuroda <[email-masked]>
Co-authored-by: Fujii Masao <[email-masked]>
Reviewed-by: Ashutosh Bapat <[email-masked]>
Reviewed-by: Vignesh C <[email-masked]>
Reviewed-by: David G. Johnston <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 doc/src/sgml/ref/pg_recvlogical.sgml | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

+       <para>

+       <para>

From 44c686ae11cc0de8d0aaf70c23397f94b477a82a Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email-masked]>
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-masked]>
Reviewed-by: Fujii Masao <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 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)
 
 	/*
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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 24 Mar 2025 02:21:43 +0000

Dear Fujii-san,

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

Thanks for updating the patch. LGTM.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Fujii Masao <[email-masked]>
Date: Tue, 25 Mar 2025 00:56:34 +0900

> Thanks for updating the patch. LGTM.

I've pushed the patches. Thanks!

Regards,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 25 Mar 2025 02:10:55 +0000

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