pg_recvlogical requires -d but not described on the documentation
Dear hackers,
Hi, I found the issue $SUBJECT. According to the doc [1]https://www.postgresql.org/docs/devel/app-pgrecvlogical.html:
```
-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
Attachments:
0001-pg_recvlogical-accept-if-d-is-not-specified.patchapplication/octet-stream; name=0001-pg_recvlogical-accept-if-d-is-not-specified.patchDownload
From d12206817f78721b7739389c23277f959a61c263 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Fri, 21 Feb 2025 13:22:37 +0900
Subject: [PATCH] pg_recvlogical: accept if -d is not specified
---
src/bin/pg_basebackup/pg_recvlogical.c | 20 ++++++++++++-------
src/bin/pg_basebackup/t/030_pg_recvlogical.pl | 4 ++--
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index b9ea23e142..3fbc2e819e 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -20,6 +20,7 @@
#include "common/file_perm.h"
#include "common/logging.h"
+#include "common/username.h"
#include "fe_utils/option_utils.h"
#include "getopt_long.h"
#include "libpq-fe.h"
@@ -864,6 +865,18 @@ main(int argc, char **argv)
exit(1);
}
+ if (dbname == NULL)
+ {
+ if (getenv("PGDATABASE"))
+ dbname = getenv("PGDATABASE");
+ else if (dbuser)
+ dbname = dbuser;
+ else if (getenv("PGUSER"))
+ dbname = getenv("PGUSER");
+ else
+ dbname = (char *) get_user_name_or_exit(progname);
+ }
+
/*
* Required arguments
*/
@@ -881,13 +894,6 @@ main(int argc, char **argv)
exit(1);
}
- if (!do_drop_slot && dbname == NULL)
- {
- pg_log_error("no database specified");
- pg_log_error_hint("Try \"%s --help\" for more information.", progname);
- exit(1);
- }
-
if (!do_drop_slot && !do_create_slot && !do_start_slot)
{
pg_log_error("at least one action needs to be specified");
diff --git a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl
index a6e1060016..6fccdba63c 100644
--- a/src/bin/pg_basebackup/t/030_pg_recvlogical.pl
+++ b/src/bin/pg_basebackup/t/030_pg_recvlogical.pl
@@ -29,8 +29,8 @@ $node->start;
$node->command_fails(['pg_recvlogical'], 'pg_recvlogical needs a slot name');
$node->command_fails(
- [ 'pg_recvlogical', '--slot' => 'test' ],
- 'pg_recvlogical needs a database');
+ [ 'pg_recvlogical', '--slot' => 'test', '--user' => 'postgres' ],
+ 'pg_recvlogical needs an action');
$node->command_fails(
[ 'pg_recvlogical', '--slot' => 'test', '--dbname' => 'postgres' ],
'pg_recvlogical needs an action');
--
2.43.5
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
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 nameIf 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:
0001-pg_recvlogical-Make-sure-d-is-mandatory-option.patchapplication/octet-stream; name=0001-pg_recvlogical-Make-sure-d-is-mandatory-option.patchDownload
From 342690e8af038578bbedf834caae85bcdbc2a5e0 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
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
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 nameIf 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
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
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.
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 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
Attachments:
v2-0001-doc-Clarify-required-options-for-each-action-in-p.patchtext/plain; charset=UTF-8; name=v2-0001-doc-Clarify-required-options-for-each-action-in-p.patchDownload
From 8dcc53a6bebeaa1c4d0ddaacd35a3c450327bfef Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
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
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.
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
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
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]/messages/by-id/OSCPR01MB14966C6BE304B5BB2E58D4009F5DE2@OSCPR01MB14966.jpnprd01.prod.outlook.com.
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]: /messages/by-id/OSCPR01MB14966C6BE304B5BB2E58D4009F5DE2@OSCPR01MB14966.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
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:
v3-0001-doc-Clarify-required-options-for-each-action-in-p.patchapplication/octet-stream; name=v3-0001-doc-Clarify-required-options-for-each-action-in-p.patchDownload
From 959222ee6d2f83308324253bcbe3add3466b9533 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
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
v3-0002-pg_recvlogical-allow-working-without-dbname-while.patchapplication/octet-stream; name=v3-0002-pg_recvlogical-allow-working-without-dbname-while.patchDownload
From 75e0b8649c7370132919e95870cd712bfc33020d Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
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
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
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
Attachments:
v4-0001-doc-Clarify-required-options-for-each-action-in-p.patchtext/plain; charset=UTF-8; name=v4-0001-doc-Clarify-required-options-for-each-action-in-p.patchDownload
From 53360ffb7a908a881a94ff81934dca9a7825d8d1 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
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 <kuroda.hayato@fujitsu.com>
Co-authored-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Reviewed-by: Vignesh C <vignesh21@gmail.com>
Reviewed-by: David G. Johnston <david.g.johnston@gmail.com>
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
v4-0002-Allow-pg_recvlogical-drop-slot-to-work-without-db.patchtext/plain; charset=UTF-8; name=v4-0002-Allow-pg_recvlogical-drop-slot-to-work-without-db.patchDownload
From 44c686ae11cc0de8d0aaf70c23397f94b477a82a Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
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 <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: https://postgr.es/m/b15ecf4f-e5af-4fbb-82c2-a425f453e0b2@oss.nttdata.com
---
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
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
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]/messages/by-id/CAKFQuwYWVT84GM2OqRx8EqNrfzNM-zbpQ5Y2bA1dPO9jUgo_Kg@mail.gmail.com, we would discuss the manner for documentations in another thread.
I've closed the CF entry as "committed". Thanks!
[1]: /messages/by-id/CAKFQuwYWVT84GM2OqRx8EqNrfzNM-zbpQ5Y2bA1dPO9jUgo_Kg@mail.gmail.com
Best regards,
Hayato Kuroda
FUJITSU LIMITED