public inbox for [email protected]
help / color / mirror / Atom feedFrom: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Paul A Jungwirth' <[email protected]>
To: Mutaamba Maasha <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Bertrand Drouvot <[email protected]>
Cc: [email protected] <[email protected]>
Subject: RE: ReplicationSlotRelease() crashes when the instance is in the single user mode
Date: Mon, 18 Aug 2025 08:47:27 +0000
Message-ID: <OSCPR01MB14966B39B3ED21229EAEFA10FF531A@OSCPR01MB14966.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CA+renyWi+bbD8M8CDAaOMGbUrXptKLCK8zQWEhXy00PJuJqNuQ@mail.gmail.com>
References: <OSCPR01MB14966ED588A0328DAEBE8CB25F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB149669EF7D087A588773B7E04F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966559522B9699E1F769128F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OSCPR01MB14966F234CD3121F51810D5CCF5C52@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<[email protected]>
<OSCPR01MB14966FCE16ECCF29AE0502AF2F5C42@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<OSCPR01MB149660520A0E53CC53E8B66F1F5C42@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAA4eK1LK5nZUWwNY31X6BUXdQwwzpKGKxnw+zFf-cWKm3nuCEg@mail.gmail.com>
<OSCPR01MB149669CFD736D48C4450337DDF5CD2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAA4eK1L8_QrYavkCy9b0Rrz8gqZRQE8kEmv3g7Tz6tsN8YuvhA@mail.gmail.com>
<OSCPR01MB149660CD4D6648CFE7FE68D34F5CC2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CA+renyWi+bbD8M8CDAaOMGbUrXptKLCK8zQWEhXy00PJuJqNuQ@mail.gmail.com>
Dear Paul, Mutaamba,
Here are updated patches. Based on the Robert's suggestion, I separated into two parts.
0001 fixed the original issue and 0002 prohibited the slot manipulation in
single-user mode. I want to focus on 0001 first because on one would argue it.
All comments from you were included in 0002.
> The patch does not apply. The attached v5 fixes it with a small update
> to the context for the slot.h hunk.
Oh, I didn't realize because cfbot said OK. Anyway, new patch could be applied atop HEAD.
> The commit message needs some explanation. Why are we doing this? What
> does the patch do? What alternatives did we consider?
Updated. How do you feel?
> The patch has no docs. If we plan to forbid some functions in
> single-user mode, we should document which ones (e.g. in func.sgml).
A statement was added.
> There are no tests. Do we have any tests at all for single-user mode?
> The only one I see is in recovery/t/017_shm.pl. What if we had tests
> that ran the regress suite in single-user mode? Basically instead of
> psql we run postgres --single? Do we expect a lot of it would fail? Is
> it small enough the test could maintain a diff that expects those
> failures? I'm not saying we should do that as part of this patch, but
> is there any interest in that? Since single-user mode is most often
> used when things are broken, it's a harsh place to hit a bug.
>
> The patch lacks source comments. Something on
> CheckSlotIsInSingleUserMode explaining why would be good.
Comments were added in all caller functions.
> In ReplicationSlotRelease, why only assert that `slot->active_pid !=
> 0`? Why not assert that it's MyProcPid (even outside single-user
> mode)? Can one backend really release a slot while another is using
> it? Can you drop it? Maybe you can copy it?
You meant we should assert `slot->active_pid == MyProcPid`, right?
Naively considered it can be fix, but it is out-of-scope of the thread. It is
existing code, should be discussed verified in another place.
> Are we calling CheckSlotIsInSingleUserMode from everywhere that needs
> it? We tried to check all the functions that call
> ReplicationSlotCreate, ReplicationSlotRelease, and
> update_synced_slots_inactive_since (since they all have these
> asserts). To call out a few:
>
> The pg_replication_origin_* functions don't call the Assert-ing functions.
You asked that whether we should call CheckSlotIsInSingleUserMode in
pg_replication_origin_* APIs, right? It depends on the policy. If we want to
prohibit all replication-related command, it should be. It is still under
discussion, so I did not touch.
> binary_upgrade_logical_slot_has_caught_up - Not available from the command
> line.
I found that we can in both single-user and binary-upgrade mode. At that time
the function could be called:
```
$ postgres --single -D data/ postgres -b
PostgreSQL stand-alone backend 19devel
backend> SELECT binary_upgrade_logical_slot_has_caught_up('test');
1: binary_upgrade_logical_slot_has_caught_up (typeid = 16, len = 1, typmod = -1, byval = t)
----
LOG: starting logical decoding for slot "test"
DETAIL: Streaming transactions committing after 0/0182C640, reading WAL from 0/0182C608.
STATEMENT: SELECT binary_upgrade_logical_slot_has_caught_up('test');
LOG: logical decoding found consistent point at 0/0182C608
DETAIL: There are no running transactions.
STATEMENT: SELECT binary_upgrade_logical_slot_has_caught_up('test');
1: binary_upgrade_logical_slot_has_caught_up = "f" (typeid = 16, len = 1, typmod = -1, byval = t)
----
```
I don't think this is a realistic case, but the check was added.
> WalSndErrorCleanup - Probably not used in single-user mode?
IIUC we won't reach in the single-user mode. Hence it was not called intentionally.
> We also see that PostgresMain calls ReplicationSlotRelease from its
> error handler. Presumably single-user mode would be executing
> PostgresSingleUserMain instead,
ISTM, PostgresMain() would be called from the PostgresSingleUserMain().
> but still perhaps we should call
> CheckSlotIsInSingleUserMode here just as a sanity-check?
I feel this code is to release the acquired slot when ERROR was raised. Since we
have already covered the entrance, it is not needed.
[1]: https://www.postgresql.org/message-id/CA%2BTgmobspWhoMysNHL9b3C9xi%3DOpHdBYhtAgDH4N_A2foyjN-w%40mail...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v5-0001-Set-ReplicationSlot-active_pid-even-in-single-use.patch (929B, ../OSCPR01MB14966B39B3ED21229EAEFA10FF531A@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v5-0001-Set-ReplicationSlot-active_pid-even-in-single-use.patch)
download | inline diff:
From 201b872dbcdfb8a1182edde13b14a24d5aa43226 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Mon, 18 Aug 2025 11:47:43 +0900
Subject: [PATCH v5 1/2] Set ReplicationSlot::active_pid even in single-user
mode
Slot manipulation functions except drop could be failed in single-user mode, because
active_pid was not set. Actually the attribute is no-op in the mode but fix the
inconsisntency anyway.
---
src/backend/replication/slot.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 8605776ad86..fd0fdb96d42 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -653,7 +653,7 @@ retry:
}
else
{
- active_pid = MyProcPid;
+ s->active_pid = active_pid = MyProcPid;
ReplicationSlotSetInactiveSince(s, 0, true);
}
LWLockRelease(ReplicationSlotControlLock);
--
2.47.1
[application/octet-stream] v5-0002-Prohibit-slot-manipulation-while-in-single-user-m.patch (5.8K, ../OSCPR01MB14966B39B3ED21229EAEFA10FF531A@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v5-0002-Prohibit-slot-manipulation-while-in-single-user-m.patch)
download | inline diff:
From 3ac78c7a7a42afbbc9cb09c03b568c1c39cd035c Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email protected]>
Date: Wed, 19 Feb 2025 11:37:26 +0900
Subject: [PATCH v5 2/2] Prohibit slot manipulation while in single-user mode
Replication-related commands are rarely used in single-user mode and have been
broken for years. This commit prohibits calling slot manipulation SQL functions
to avoid additional risk of failures. One exception is pg_drop_replication_slot.
It is still allowed because users may want to clean up their mistakes in the mode.
---
doc/src/sgml/func/func-admin.sgml | 5 +++++
src/backend/replication/logical/logicalfuncs.c | 3 +++
src/backend/replication/slot.c | 12 ++++++++++++
src/backend/replication/slotfuncs.c | 17 +++++++++++++++++
src/backend/utils/adt/pg_upgrade_support.c | 3 +++
src/include/replication/slot.h | 1 +
6 files changed, 41 insertions(+)
diff --git a/doc/src/sgml/func/func-admin.sgml b/doc/src/sgml/func/func-admin.sgml
index 446fdfe56f4..cfa09e4b987 100644
--- a/doc/src/sgml/func/func-admin.sgml
+++ b/doc/src/sgml/func/func-admin.sgml
@@ -1012,6 +1012,11 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
are also relevant for replication.
</para>
+ <para>
+ Note that slot manipulation functions except <function>pg_drop_replication_slot</function>
+ cannot be used in single-user mode.
+ </para>
+
<table id="functions-replication-table">
<title>Replication Management Functions</title>
<tgroup cols="1">
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index ca53caac2f2..30877fdc3ab 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -113,6 +113,9 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
List *options = NIL;
DecodingOutputState *p;
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
CheckLogicalDecodingRequirements();
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index fd0fdb96d42..4ca1227cc94 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1529,6 +1529,18 @@ CheckSlotPermissions(void)
"REPLICATION")));
}
+/*
+ * Check whether the instance is in single-user mode.
+ */
+void
+CheckSlotIsInSingleUserMode(void)
+{
+ if (!IsUnderPostmaster)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("replication slots cannot be used in single-user mode")));
+}
+
/*
* Reserve WAL for the currently active slot.
*
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 69f4c6157c5..4de2e52fba9 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -17,6 +17,7 @@
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
#include "funcapi.h"
+#include "miscadmin.h"
#include "replication/logical.h"
#include "replication/slot.h"
#include "replication/slotsync.h"
@@ -76,6 +77,9 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
CheckSlotRequirements();
@@ -182,6 +186,9 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
CheckLogicalDecodingRequirements();
@@ -521,6 +528,9 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
Assert(!MyReplicationSlot);
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
if (XLogRecPtrIsInvalid(moveto))
@@ -618,9 +628,13 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
TupleDesc tupdesc;
HeapTuple tuple;
+
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
if (logical_slot)
@@ -898,6 +912,9 @@ pg_sync_replication_slots(PG_FUNCTION_ARGS)
char *err;
StringInfoData app_name;
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
CheckSlotPermissions();
if (!RecoveryInProgress())
diff --git a/src/backend/utils/adt/pg_upgrade_support.c b/src/backend/utils/adt/pg_upgrade_support.c
index a4f8b4faa90..337f705b34b 100644
--- a/src/backend/utils/adt/pg_upgrade_support.c
+++ b/src/backend/utils/adt/pg_upgrade_support.c
@@ -296,6 +296,9 @@ binary_upgrade_logical_slot_has_caught_up(PG_FUNCTION_ARGS)
*/
Assert(has_rolreplication(GetUserId()));
+ /* Slot manipulation is not allowed in single-user mode */
+ CheckSlotIsInSingleUserMode();
+
slot_name = PG_GETARG_NAME(0);
/* Acquire the given slot */
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index e8fc342d1a9..b4d89760f96 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -342,6 +342,7 @@ extern void CheckPointReplicationSlots(bool is_shutdown);
extern void CheckSlotRequirements(void);
extern void CheckSlotPermissions(void);
+extern void CheckSlotIsInSingleUserMode(void);
extern ReplicationSlotInvalidationCause
GetSlotInvalidationCause(const char *cause_name);
extern const char *GetSlotInvalidationCauseName(ReplicationSlotInvalidationCause cause);
--
2.47.1
view thread (31+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: RE: ReplicationSlotRelease() crashes when the instance is in the single user mode
In-Reply-To: <OSCPR01MB14966B39B3ED21229EAEFA10FF531A@OSCPR01MB14966.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox