doc patch: wrong descriptions for dropping replication slots

Started by Hayato Kuroda (Fujitsu)10 months ago6 messages
#1Hayato Kuroda (Fujitsu)
kuroda.hayato@fujitsu.com
1 attachment(s)

Dear hackers,

While considering another thread, I found the $SUBJECT. Attached patch fixes it.

Documentation says:

```
pg_drop_replication_slot ( slot_name name ) → void

Drops the physical or logical replication slot named slot_name. Same as replication protocol command DROP_REPLICATION_SLOT.
For logical slots, this must be called while connected to the same database the slot was created on.
```

But this is not correct. Backend processes which connect to other databases
can drop the logical slot:

```
postgres=# SELECT * FROM pg_create_logical_replication_slot('test', 'test_decoding');
slot_name | lsn
-----------+-----------
test | 0/1CA6A18
(1 row)

postgres=# \c tests
You are now connected to database "tests" as user "postgres".
tests=# SELECT * FROM pg_drop_replication_slot('test');
pg_drop_replication_slot
--------------------------

(1 row)
```

IIUC, the description was added by ff539d. The initial version [1]/messages/by-id/CAMsr+YGjZRqo-boCF9z5Bc1WZ_10RjMLtNSTsaa=kkE9_GmTag@mail.gmail.com seemed to have
the restriction, it was removed now but the description retained.

I think all supported versions have the same issue, attached one is for master.

Thanks Hou for confirming the issue.

[1]: /messages/by-id/CAMsr+YGjZRqo-boCF9z5Bc1WZ_10RjMLtNSTsaa=kkE9_GmTag@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

Attachments:

0001-Fix-description-for-dropping-slots.patchapplication/octet-stream; name=0001-Fix-description-for-dropping-slots.patchDownload
From c986025233b6f698526d0aa0ac7d041845489568 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 18 Mar 2025 17:45:04 +0900
Subject: [PATCH] Fix description for dropping slots

---
 doc/src/sgml/func.sgml     | 3 +--
 doc/src/sgml/protocol.sgml | 2 --
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 1c3810e1a04..2b76bb3ddcb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -29452,8 +29452,7 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
        <para>
         Drops the physical or logical replication slot
         named <parameter>slot_name</parameter>. Same as replication protocol
-        command <literal>DROP_REPLICATION_SLOT</literal>. For logical slots, this must
-        be called while connected to the same database the slot was created on.
+        command <literal>DROP_REPLICATION_SLOT</literal>.
        </para></entry>
       </row>
 
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 3bd9e68e6ce..04d8e7d21af 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -2802,8 +2802,6 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
      <listitem>
       <para>
        Drops a replication slot, freeing any reserved server-side resources.
-       If the slot is a logical slot that was created in a database other than
-       the database the walsender is connected to, this command fails.
       </para>
 
       <variablelist>
-- 
2.43.5

#2Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Hayato Kuroda (Fujitsu) (#1)
Re: doc patch: wrong descriptions for dropping replication slots

On 2025/03/18 17:46, Hayato Kuroda (Fujitsu) wrote:

Dear hackers,

While considering another thread, I found the $SUBJECT. Attached patch fixes it.

Thanks for the patch!

Documentation says:

```
pg_drop_replication_slot ( slot_name name ) → void

Drops the physical or logical replication slot named slot_name. Same as replication protocol command DROP_REPLICATION_SLOT.
For logical slots, this must be called while connected to the same database the slot was created on.
```

But this is not correct. Backend processes which connect to other databases
can drop the logical slot:

```
postgres=# SELECT * FROM pg_create_logical_replication_slot('test', 'test_decoding');
slot_name | lsn
-----------+-----------
test | 0/1CA6A18
(1 row)

postgres=# \c tests
You are now connected to database "tests" as user "postgres".
tests=# SELECT * FROM pg_drop_replication_slot('test');
pg_drop_replication_slot
--------------------------

(1 row)
```

IIUC, the description was added by ff539d. The initial version [1] seemed to have
the restriction, it was removed now but the description retained.

Why was this restriction removed? If there was a past discussion about it,
could you share the details?

Since it's generally expected that a session in one database shouldn't
be able to drop objects in another, I'm wondering if removing this
restriction was intentional or possibly a bug.

Regards,

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

#3Hayato Kuroda (Fujitsu)
kuroda.hayato@fujitsu.com
In reply to: Fujii Masao (#2)
RE: doc patch: wrong descriptions for dropping replication slots

Dear Fujii-san,

Why was this restriction removed? If there was a past discussion about it,
could you share the details?

More properly, pg_drop_replication_slot() has been introduced since PG9.4, and old
documents did not have the description. The description has been added while
developing PG10 and kept till now.

The restriction was introduced in v1 patch and removed in v2 patch [1]/messages/by-id/CAMsr+YEsrUf0sq9RtK7cy2wM7+pUQQ22LzPU-FzVOBL7PROwhg@mail.gmail.com.
ISTM there were no discussions in the thread. I tried to find the initial design
of the function, but I could not find.

Since it's generally expected that a session in one database shouldn't
be able to drop objects in another, I'm wondering if removing this
restriction was intentional or possibly a bug.

I think the description was accidentally retained. As I said above, replication slot
can be dropped from anywhere since PG9.4. Andres pointed out the description was
not needed in post-commit review [2]/messages/by-id/20170328152238.h3ikwwsl5kbqq6nk@alap3.anarazel.de. Craig posted a follow-up patch [3]/messages/by-id/CAMsr+YFdRMxFNvofWyBJ1zdhFgEBHZf=0TWrt9Z3QRSWLepvgQ@mail.gmail.com, but it was
missed.

[1]: /messages/by-id/CAMsr+YEsrUf0sq9RtK7cy2wM7+pUQQ22LzPU-FzVOBL7PROwhg@mail.gmail.com
[2]: /messages/by-id/20170328152238.h3ikwwsl5kbqq6nk@alap3.anarazel.de
[3]: /messages/by-id/CAMsr+YFdRMxFNvofWyBJ1zdhFgEBHZf=0TWrt9Z3QRSWLepvgQ@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

#4Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Hayato Kuroda (Fujitsu) (#3)
Re: doc patch: wrong descriptions for dropping replication slots

On 2025/03/19 11:07, Hayato Kuroda (Fujitsu) wrote:

Dear Fujii-san,

Why was this restriction removed? If there was a past discussion about it,
could you share the details?

More properly, pg_drop_replication_slot() has been introduced since PG9.4, and old
documents did not have the description. The description has been added while
developing PG10 and kept till now.

The restriction was introduced in v1 patch and removed in v2 patch [1].
ISTM there were no discussions in the thread. I tried to find the initial design
of the function, but I could not find.

Since it's generally expected that a session in one database shouldn't
be able to drop objects in another, I'm wondering if removing this
restriction was intentional or possibly a bug.

I think the description was accidentally retained. As I said above, replication slot
can be dropped from anywhere since PG9.4. Andres pointed out the description was
not needed in post-commit review [2]. Craig posted a follow-up patch [3], but it was
missed.

Thanks for the clarification! I agree that the description is incorrect
and should be removed.

Unless there are any objections, I plan to push your patch with
the following commit message and back-patch it to all supported versions.

-------
doc: Remove incorrect description about dropping replication slots.

pg_drop_replication_slot() can drop replication slots created on
a different database than the one where it is executed. This behavior
has been in place since PostgreSQL 9.4, when pg_drop_replication_slot()
was introduced.

However, commit ff539d mistakenly added the following incorrect
description in the documentation:

For logical slots, this must be called when connected to
the same database the slot was created on.

This commit removes that incorrect statement. A similar mistake was
also present in the documentation for the DROP_REPLICATION_SLOT
command, which has now been corrected as well.

Back-patch to all supported versions.

Author: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Discussion: /messages/by-id/OSCPR01MB14966C6BE304B5BB2E58D4009F5DE2@OSCPR01MB14966.jpnprd01.prod.outlook.com
Backpatch-through: 13
-------

Regards,

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

#5Hayato Kuroda (Fujitsu)
kuroda.hayato@fujitsu.com
In reply to: Fujii Masao (#4)
RE: doc patch: wrong descriptions for dropping replication slots

Dear Fujii-san,

Unless there are any objections, I plan to push your patch with
the following commit message and back-patch it to all supported versions.

...

Thanks for updating the commit message. LGTM.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

#6Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Hayato Kuroda (Fujitsu) (#5)
Re: doc patch: wrong descriptions for dropping replication slots

On 2025/03/21 10:07, Hayato Kuroda (Fujitsu) wrote:

Dear Fujii-san,

Unless there are any objections, I plan to push your patch with
the following commit message and back-patch it to all supported versions.

...

Thanks for updating the commit message. LGTM.

I've committed the patch with that message. Thanks!

Regards,

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