Subject: ReplicationSlotRelease() crashes when the instance is in the single user mode

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

Dear hackers,

I found $SUBJECT when I'm playing with the single user mode.

How to reproduce
===========
You can reproduce the failure with below steps.

```
# Initialize an instance
$ initdb -D data -U postgres
# Start it as single user mode
$ postgres --single -D data/ postgres 

PostgreSQL stand-alone backend 18devel
backend> SELECT pg_create_physical_replication_slot(slot_name := 'physical_slot', immediately_reserve := true);
...
backend> SELECT pg_replication_slot_advance('physical_slot', pg_current_wal_lsn());
         1: pg_replication_slot_advance (typeid = 2249, len = -1, typmod = -1, byval = f)
        ----
TRAP: failed Assert("slot != NULL && (slot->active_pid != 0)"), File: "../postgres/src/backend/replication/slot.c", Line: 674, PID: 430860
postgres(ExceptionalCondition+0xab)[0xb86a2a]
postgres(ReplicationSlotRelease+0x5a)[0x8df10b]
postgres(pg_replication_slot_advance+0x330)[0x8e46ed]
...
```

Analysis
=====
We trapped at below assertion in ReplicationSlotRelease(). IIUC, `slot->active_pid` is set
only when the process is under the postmaster, but ReplicationSlotRelease() always requires it.

```
	Assert(slot != NULL && slot->active_pid != 0);
```

Possible fix
=======

Naively considered, there are two approaches to fix this. 1) set active_pid when even in the single
user mode [1], or 2) ease the condition to accept the situation [2]. I'm not familiar with the mode,
but [1] seems better if we want to unify codes.

Thought?

[1]:
```
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -599,7 +599,7 @@ retry:
                SpinLockRelease(&s->mutex);
        }
        else
-               active_pid = MyProcPid;
+               s->active_pid = active_pid = MyProcPid;
        LWLockRelease(ReplicationSlotControlLock);
 
        /*
```
[2]:
```
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -671,7 +671,8 @@ ReplicationSlotRelease(void)
        bool            is_logical = false; /* keep compiler quiet */
        TimestampTz now = 0;
 
-       Assert(slot != NULL && slot->active_pid != 0);
+       Assert(slot != NULL &&
+                  (slot->active_pid != 0 || !IsUnderPostmaster));
 
        if (am_walsender)
        {
```

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Tue, 18 Feb 2025 15:15:48 +0900

> I found $SUBJECT when I'm playing with the single user mode.

Perhaps a very naive question, but is there any point in authorizing
manipulations of MyReplicationSlot in single-user mode, to begin with?
With this remark, I would mean to apply a rule to
ReplicationSlotAcquire(), so as all its callers would know about that.
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 18 Feb 2025 07:29:51 +0000

Dear Michael,

> ReplicationSlotAcquire(), so as all its callers would know about that.

According to the original thread [1], there was a wide consensus replication-related
operations can be rejected, except the slot removal. I feel this is reasonable.

Currently pg_drop_replication_slot() requires the droping slot can be acquired,
so we cannot reject it in single user mode as-is. Maybe we should revive the 0002
patch in [1] then try to do that. Thought?

[1]: https://www.postgresql.org/message-id/3b2f809f-326c-38dd-7a9e-897f957a4eb1%40enterprisedb.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Tue, 18 Feb 2025 16:50:22 +0900

> [1]: https://www.postgresql.org/message-id/3b2f809f-326c-38dd-7a9e-897f957a4eb1%40enterprisedb.com

Ah, good point for the slot drop.  So 0ce5cf2ef24f is claiming that
some of these manipulations are OK.  I didn't suspect this one.

Slot advancing is a very different beast, unfortunately, that may
depend on many other subsystems.  For example with logical slots we
would finish by calling rm_decode, which could be outside of core.
Justifying that this operation is supported in single-user mode is
larger than what you are suggesting here..
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 18 Feb 2025 08:23:43 +0000

Dear Michael,

> some of these manipulations are OK.  I didn't suspect this one.

Yeah, I think so.

> larger than what you are suggesting here..

OK. Actually, I do not have any use-cases in my mind. I played because
I found a special path for single user mode in ReplicationSlotAcquire()
so that I wanted to see the behavior.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 19 Feb 2025 02:57:34 +0000

Dear hackers,

Based on the discussion, I feel it is enough to add quick error out for SQL functions. PSA attached.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

> -----Original Message-----
> From: Hayato Kuroda (Fujitsu) <[email-masked]>
> Sent: Tuesday, February 18, 2025 5:24 PM
> To: 'Michael Paquier' <[email-masked]>
> Cc: [email-masked]' <[email-masked]>
> Subject: RE: ReplicationSlotRelease() crashes when the instance is in the single
> user mode
> 
> Dear Michael,
>

---

From: Michael Paquier <[email-masked]>
Date: Thu, 20 Feb 2025 09:02:19 +0900

> for SQL functions. PSA attached.

I did not check how these call behave individually, just a few
comments while putting my eyes on the patch.

+	if (!IsUnderPostmaster)
+		elog(ERROR,
+			 "slot operation is prohibited in the single user mode");

elog() should not be used for failures that can be user-facing as this
would not provide any translation.

I'd suggest rewording the error message to provide some more context,
as well, say:
"cannot use %s in single-user mode", "function_name"
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 20 Feb 2025 02:22:41 +0000

Dear Michael,

> would not provide any translation.

I intentionally used elog() because I thought single user mode is not user-facing.
But it is OK for me to use ereport() instead.

> "cannot use %s in single-user mode", "function_name"

Fixed. PSA new version

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 19 Feb 2025 19:27:11 -0700

> But it is OK for me to use ereport() instead.

Single-user mode is also known as "Oh crap!" mode, something used when
starting the server in multi-user model fails for non-trivial reasons.

It is also what at least one PostgreSQL Online Service (a.k.a. fiddle) uses
as an implementation choice.

David J.

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 20 Feb 2025 10:29:56 +0000

Hi,

> Dear Michael,

Thanks for the report and the patch!

> But it is OK for me to use ereport() instead.

Yeah, I think it's also about using ereport for cases that we think can happen (
and so needs to be translated). In this case, it clearly can happen so ereport()
is better.

> > "cannot use %s in single-user mode", "function_name"

=== 1

Nit:

"cannot use function %s in single-user mode", "function_name"" maybe? (that would
somehow match the existing ""user-defined types cannot use subscripting function %s")

=== 2

+ "pg_copy_replication_slot")));

s/pg_copy_replication_slot/copy_replication_slot/ maybe? As it is the function
that would report the error actually (could be called from
pg_copy_logical_replication_slot_[a|b|c] and pg_copy_physical_replication_slot_[a|b]
though).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 20 Feb 2025 12:51:17 +0000

Dear hackers,

Thanks everyone for giving comments! PSA new version.
What's new:

- Message format was modified to {"cannot use function %s in single-user mode", "function_name"}
- Reporting funcname was adjusted based on the parameters. ternary operator was used.
- Guard was added for functions in logicalfunction.c.

For now, functions for replication origin and replication messages were retained.
I can handle them after the discussion.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Sat, 22 Feb 2025 10:28:28 +0530

> - Guard was added for functions in logicalfunction.c.

Shouldn't such a check be present in the CheckSlotPermissions() kind
of function to perform it in the central place?

> I can handle them after the discussion.

Which other functions do we see similar restrictions? I checked
"sequence manipulation functions" (1), and "Transaction ID and
Snapshot Information Functions" (2) but couldn't see similar
restrictions.

(1) - https://www.postgresql.org/docs/current/functions-sequence.html
(2) - https://www.postgresql.org/docs/current/functions-info.html#FUNCTIONS-INFO-SNAPSHOT

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 27 Feb 2025 07:58:40 +0000

Dear Amit,

> of function to perform it in the central place?

OK. I checked whether we can reuse pre-existing functions, but it seems not appropriate.
CheckSlotPermissions() is called even by pg_drop_replication_slot(), and
CheckSlotRequirements() is not called by pg_sync_replication_slots(). I defined new
function CheckSlotIsInSingleUserMode() and put the check there.
I removed function name from the ereport(), but I feel it is enough - CheckSlotPermissions()
and CheckSlotRequirements() do not have.

> (2) -
> https://www.postgresql.org/docs/current/functions-info.html#FUNCTIONS-INF
> O-SNAPSHOT

I grepped sources and could not find explicit limitations neither. So...this might
be overkill.　But I still think the restriction is OK for the slot - no need to do
some efforts for accepting single-user mode, just close the cover.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Fri, 28 Feb 2025 08:43:02 +0530

On Thu, Feb 27, 2025 at 1:29 PM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> be overkill.

Yes, this is my worry.

>　But I still think the restriction is OK for the slot - no need to do

I understand that we may not have a clear use case for this to work in
single-user mode. But how will we define the boundary in similar
cases? I mean, we should have some rule for such exposed functions,
and it should be followed uniformly. Now, if one needs a bigger or
complex change to make the function work in single-user mode, then it
is easy to take an exception from what is currently being followed in
the code. However, if the change is as trivial as you proposed in the
first email, why not go with that and make this function work in
single-user mode?

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 28 Feb 2025 05:41:59 +0000

Dear Amit,

> first email, why not go with that and make this function work in

Hmm, the opinion about this is completely opposite with other reviewers. I want
to ask them again how you feel. I also added Tom who pointed out in the initial
thread.

Question: how you feel to restrict SQL functions for slot during the single-user
mode? Nobody has considered use cases for it; we do not have concrete theory and
similar cases. And needed band-aid to work seems very small.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Paul A Jungwirth <[email-masked]>
Date: Tue, 5 Aug 2025 09:51:16 -0700

Mutaamba (cc'd) and I reviewed this patch together.

To summarize the patch and thread so far: The patch adds a new
function, CheckSlotIsInSingleUserMode. If true then we raise an error.
Otherwise we would trip an assert in ReplicationSlotRelease requiring
the slot to have an active_pid, which is never set in single-user
mode. We do want to support pg_drop_replication_slot in single-user
mode, but not other replication slot functions. In particular,
advancing the slot may call non-core code, which seems risky in
single-user mode.

The patch does not apply. The attached v5 fixes it with a small update
to the context for the slot.h hunk.

The commit message needs some explanation. Why are we doing this? What
does the patch do? What alternatives did we consider?

The current error message seems reasonable.

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

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.

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?

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.

binary_upgrade_logical_slot_has_caught_up - Not available from the command line.

WalSndErrorCleanup - Probably not used in single-user mode?

We also see that PostgresMain calls ReplicationSlotRelease from its
error handler. Presumably single-user mode would be executing
PostgresSingleUserMain instead, but still perhaps we should call
CheckSlotIsInSingleUserMode here just as a sanity-check?

> similar cases. And needed band-aid to work seems very small.

No one has replied yet, but I vote for forbidding these functions. I
can't articulate a full theory for which functions we restrict in
single-user mode, and I think we should permit as much as possible.
But any theory would weigh usefulness against risk. Here no one has
found a use-case for several years, and executing user-supplied code
in an unusual, higher-risk scenario seems like a good reason to be
cautious. Also without tests for single-user mode, I'm extra wary. If
single-user support is desired by someone, they could submit a patch.

Yours,

--
Paul              ~{:-)
[email-masked]
From b19815119909d0529d6c52889ab0959ec44e7e7f Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <[email-masked]>
Date: Wed, 19 Feb 2025 11:37:26 +0900
Subject: [PATCH v4] Prohibit slot operations while in the single user mode

---
 src/backend/replication/logical/logicalfuncs.c |  2 ++
 src/backend/replication/slot.c                 | 12 ++++++++++++
 src/backend/replication/slotfuncs.c            | 12 ++++++++++++
 src/include/replication/slot.h                 |  1 +
 4 files changed, 27 insertions(+)

diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index ca53caac2f..178a07af7d 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -113,6 +113,8 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
 	List	   *options = NIL;
 	DecodingOutputState *p;
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	CheckLogicalDecodingRequirements();
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 719e531eb9..d041adfe48 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1442,6 +1442,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 146eef5871..b669774c87 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,8 @@ 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");
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	CheckSlotRequirements();
@@ -182,6 +185,8 @@ 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");
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	CheckLogicalDecodingRequirements();
@@ -515,6 +520,8 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
 
 	Assert(!MyReplicationSlot);
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	if (XLogRecPtrIsInvalid(moveto))
@@ -612,9 +619,12 @@ 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");
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	if (logical_slot)
@@ -871,6 +881,8 @@ pg_sync_replication_slots(PG_FUNCTION_ARGS)
 	char	   *err;
 	StringInfoData app_name;
 
+	CheckSlotIsInSingleUserMode();
+
 	CheckSlotPermissions();
 
 	if (!RecoveryInProgress())
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index f5a24ccfbf..24523b1001 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -306,6 +306,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.43.5

---

From: Robert Haas <[email-masked]>
Date: Mon, 11 Aug 2025 14:25:25 -0400

> single-user support is desired by someone, they could submit a patch.

I don't feel good about the direction from which this patch is
attacking the problem. The original stack trace looks like this:

ReplicationSlotRelease() is called near the end of
pg_replication_slot_advance. IIUC, this means that we successfully
acquired the slot and did all the work, and then failed a sanity check
afterward. So, I see two possibilities: either it's not OK to acquire
a replication slot in single-user mode, in which case this should have
failed earlier, or it's also OK to release it, in which case this
should not have failed at all. Interestingly, I see that
ReplicationSlotAcquire() features a special case whose specific
purpose is to allow it to work in single-user mode, which makes me
rather suspect that the latter is correct. It is possible that it was
intentional that acquiring works in single-user mode and dropping
(other than via ReplicationSlotDropAcquired) does not, but so far I
see nothing in the comments suggesting that.

In any event, that inconsistency between how ReplicationSlotAcquire()
works and how ReplicationSlotRelease() works seems to me to be the
core thing that should be fixed here. If the selected fix also
requires error checks in higher-level functions, as added by this
patch, then well and good. But right now, the proposed patch isn't
trying to fix the definitional inconsistency in the underlying layer
at all, and is instead just installing guard rails to try to keep
anyone from bumping into it. That seems like a bad direction to go --
it feels like we're blocking access to potentially-useful
functionality because of an assertion failure that might just be
slightly sloppy coding.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 13 Aug 2025 07:21:13 +0000

Dear Robert, Paul, Mutaamba,

Sorry for the late reply. I was in the business trip.

> afterward.

Exactly. The slot is trying to be released after most of tasks are done.

```
#6  0x00000000008fd20b in pg_replication_slot_advance (fcinfo=0x23b4428)
    at ../postgres/src/backend/replication/slotfuncs.c:588
588             ReplicationSlotRelease();
(gdb) list
583              * advancing potentially done.
584              */
585             ReplicationSlotsComputeRequiredXmin(false);
586             ReplicationSlotsComputeRequiredLSN();
587 
588             ReplicationSlotRelease();
```

> So, I see two possibilities: either it's not OK to acquire
> rather suspect that the latter is correct.

Agreed.

> It is possible that it was

To confirm; your point is that we should firstly fix to allow acquiring/releasing
slots in the mode, then consider additional guards, is it right? Valid point.
We've not deeply considered but ideas described in [1] could be used.

> functionality because of an assertion failure that might just be

I still cannot find enough use-cases to allow manipulating slots, though.

[1]: https://www.postgresql.org/message-id/OSCPR01MB14966ED588A0328DAEBE8CB25F5FA2%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Robert Haas <[email-masked]>
Date: Wed, 13 Aug 2025 16:04:04 -0400

> slots in the mode, then consider additional guards, is it right? Valid point.

Yes.

> I still cannot find enough use-cases to allow manipulating slots, though.

The use case for single-user mode is quite limited in general, but
most things work in single-user mode unless they are something that
intrinsically can't. For example, parallel query cannot be used in
single-user mode, because there is only one process. Replication
cannot work, for the same reason. But manual slot operations can work,
so I do not think it is good to arbitrarily prohibit them. We do not
need a reason to specifically allow them; it is enough that there is
no good reason for them to be blocked.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 18 Aug 2025 08:47:27 +0000

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.

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

> does the patch do? What alternatives did we consider?

Updated. How do you feel?

> single-user mode, we should document which ones (e.g. in func.sgml).

A statement was added.

> CheckSlotIsInSingleUserMode explaining why would be good.

Comments were added in all caller functions.

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

> 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

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.

> PostgresSingleUserMain instead,

ISTM, PostgresMain() would be called from the PostgresSingleUserMain().

> but still perhaps we should call

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

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 18 Aug 2025 08:53:05 +0000

Dear Robert,

> no good reason for them to be blocked.

Thanks for the confirmation. Till now, it is mostly fifty-fifty in the community
whether we should allow the manipulation in the single-user mode.

I did split a patch to fix the initial issue [1]. Let's focus on the 0001 patch
firstly.

[1]: https://www.postgresql.org/message-id/OSCPR01MB14966B39B3ED21229EAEFA10FF531A%40OSCPR01MB14966.jpnprd01.prod.outlook.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Mon, 18 Aug 2025 18:02:30 +0900

> single-user mode. I want to focus on 0001 first because on one would argue it.

FWIW, I had my doubts at the beginning on the thread about the
use-case, but after re-reading the whole thing I am going to side with
Robert's opinion that if a fix to authorize some of the operations is
simple, then let's just authorize the case(s) and have the fix.

Echoing with Paul, what I find as critically lacking from the proposed
patches are regression tests to check and validate the behaviors you
are looking for in the patches.  Please add some in the shape of perl
commands that use direct --single commands of postgres.  We have cases
that do so currently in the tree: sepgsql and shm.  I don't see a
reason why we could not do that as well here with run_log().  It is
also possible to pass a -c exit_on_error=true to force failures.

Not having a test for the single-user case of a slot drop is of course
something that we are lacking now.  As we are playing with this area
of the code, let's add something for this case as well.
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 19 Aug 2025 10:26:22 +0000

Dear Michael, Paul, Mutaamba,

> simple, then let's just authorize the case(s) and have the fix.

OK, thanks for the clarification. Let's focus on 0001 now.

> of the code, let's add something for this case as well.

I added a first version of the test. It could work on my env (Linux).
Since I cannot come up the appropriate place, I introduced new test file in
recovery test. creating/dropping/advancing a slot, and doing a logical decoding
is tested. Since standby mode is not supported by the single-user instance,
I did not test the slot synchronization. I have not known that.

0002 was updated accordingly just in case. In the patch some functions were expected
to fail. Also, a check for pg_sync_replication_slots() was removed because we cannot call
the function in the first place.

IIUC, 0001 can be backpatched for all supported branches. I will create for them
after patch would be a good shape.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 19 Aug 2025 11:16:50 +0000

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Tue, 19 Aug 2025 20:17:49 +0900

> I added a first version of the test. It could work on my env (Linux).

You could also validate that using the CI, assuming that you have a
cloned repo on github with the CI enabled.  See src/tools/ci/README.
I use that a lot with local branches for pre-commit validations where
things could break across the board.

> I did not test the slot synchronization. I have not known that.

Hmm.  src/test/modules/test_misc/ makes more sense to me here.  (No
need to send an updated patch just for that.)

> after patch would be a good shape.

Thanks, I'll check all that tomorrow.
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 19 Aug 2025 12:52:24 +0000

Dear Michael,

> things could break across the board.

I have run the tests on my CI and found that windows cannot accept the test.
Per attached output, starting the postgres with the single-user mode was failed
because the user had admin permissions [1]. However, pg_ctl start command could be
done by the same user.
I'm not familiar with Windows, but according to your blogpost [2], Windows seems
to allows running some server command by generating the restricted context for
running postgres commands. In case of single-user mode, however, we directly run
the given command with the current user. So, there is a possibility that only
instance can boot only by pg_ctl.
Based on that, I want to skip the test on windows platform rather than modifying
the ci environment.

IIUC, other tests which uses --single cannot work on windows as well.

> need to send an updated patch just for that.)

I did not notice the directory and looked nice. Updated.

[1]:
```
# Running: postgres --single -F -c exit_on_error=true -D C:\cirrus\build/testrun/recovery/049_slots_in_single_user_mode\data/t_049_slots_in_single_user_mode_node_data/pgdata postgres
Execution of PostgreSQL by a user with administrative permissions is not
permitted.
The server must be started under an unprivileged user ID to prevent
possible system security compromises.  See the documentation for
more information on how to properly start the server.
```

[2]: https://paquier.xyz/postgresql-2/postgres-utilities-restricted-token/

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Aug 2025 15:05:17 +0900

> IIUC, other tests which uses --single cannot work on windows as well.

This has reminded me of 1a9d80282811, where I have used a trick with
pg_ctl to allow single-user mode executions to bypass the privileged
account permission failure, but I don't see what we could do here as
we should pass down commands to postgres for execution via stdin for
IPC::Run.  We don't really have anything WIN32-specific, so skipping
the tests on Windows sounds fine by me.

One thing that you have forgotten is to update EXTRA_INSTALL to add
test_decoding, or a make check in the module fails.  I have expanded a
bit more the tests, as for example we have paths based on active_pid
for temporary slots, which could matter at some point.  I have added a
few more things with physical slots.

With all that in mind, I don't really see the point of 0002.  There
was also a mention of replication origins upthread, but we don't
really have anything specific to a shared state or a validation in
this area AFAIK, so I don't think that test additions are worth it in
this case. 
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 20 Aug 2025 07:42:11 +0000

Dear Michael,

I found you've pushed 0001. Thanks!
I've considered to create patches for other branches after you said OK, but
you seemed to push all of them directly.

> the tests on Windows sounds fine by me.

OK, thanks for the confirmation.

> few more things with physical slots.

Oh, I didn't recognize because I usually use meson, and my CI said OK.
Good update!

> this area AFAIK, so I don't think that test additions are worth it in

Let me clarify your point. For now, there are no decisions to prohibit origin
manipulations. 0002 only restricts to handle slots in the single-user mode. Did
you say that we do not have to do tests to ensure these SQL functions are
prohibited in the mode?

(Since senior members in this thread are against to restrict, I'm planning to
stop working 0002 after creating a final patch for the reference purpose)

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Michael Paquier <[email-masked]>
Date: Wed, 20 Aug 2025 17:02:26 +0900

> you say that we do not have to do tests to ensure these SQL functions are

I am saying that there is little point in having tests for the origin
functions in single-user mode as these don't do anything really
fancy with global states (there's a acquired_by of course, and no
specific IsUnderPostmaster patch), contrary to the replication slots.
I am not convinced that there is any need to restrict them, either.
If somebody shows a reason to justify such a move, we could argue
about it, of course.
--
Michael

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 21 Aug 2025 06:39:47 +0000

Dear Michael and hackers,

This is a wrap-up of this thread. I'm planning to close the thread once.

> If somebody shows a reason to justify such a move, we could argue

OK, thanks for the clarification. Yes, there are no special path for
single-user mode for origins.

Now, senior members recently appeared here are not so motivated to restrict some
operations in single-user mode unless it cannot be done in principle. Based on
that, I decided to close the thread once until someone raised another point.
For the reference purpose, I attached a patch which prohibits slot manipulation
in single-user mode. Anyone can update and use the patch for any purposes.

Best regards,
Hayato Kuroda
FUJITSU LIMITED