MIME-Version: 1.0
References: 
 <OSCPR01MB14966ED588A0328DAEBE8CB25F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7QllLMq7UbyXHAD@paquier.xyz>
 <OSCPR01MB149669EF7D087A588773B7E04F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7Q7vo-wWDwWw-tO@paquier.xyz>
 <OSCPR01MB14966559522B9699E1F769128F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <OSCPR01MB14966F234CD3121F51810D5CCF5C52@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7ZxC-xL7y_b6Yrs@paquier.xyz>
 <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>
In-Reply-To: 
 <OSCPR01MB149660CD4D6648CFE7FE68D34F5CC2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
From: Paul A Jungwirth <pj@illuminatedcomputing.com>
Date: Tue, 5 Aug 2025 09:51:16 -0700
Message-ID: 
 <CA+renyWi+bbD8M8CDAaOMGbUrXptKLCK8zQWEhXy00PJuJqNuQ@mail.gmail.com>
Subject: Re: ReplicationSlotRelease() crashes when the instance is in the
 single user mode
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>,
 Michael Paquier <michael@paquier.xyz>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 Mutaamba Maasha <maasha@gmail.com>
Content-Type: multipart/mixed; boundary="0000000000001b2f9c063ba109ce"
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BrenyWi%2BbbD8M8CDAaOMGbUrXptKLCK8zQWEhXy00PJuJqNuQ%40mail.gmail.com>
Precedence: bulk

--0000000000001b2f9c063ba109ce
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 !=3D
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?

On Thu, Feb 27, 2025 at 9:42=E2=80=AFPM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
> > 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?
>
> 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 i=
nitial
> thread.
>
> Question: how you feel to restrict SQL functions for slot during the sing=
le-user
> mode? Nobody has considered use cases for it; we do not have concrete the=
ory and
> 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              ~{:-)
pj@illuminatedcomputing.com

--0000000000001b2f9c063ba109ce
Content-Type: text/x-patch; charset="US-ASCII";
	name="v5-0001-Prohibit-slot-operations-while-in-the-single-user.patch"
Content-Disposition: attachment;
	filename="v5-0001-Prohibit-slot-operations-while-in-the-single-user.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_mdyrz5en0>
X-Attachment-Id: f_mdyrz5en0

RnJvbSBiMTk4MTUxMTk5MDlkMDUyOWQ2YzUyODg5YWIwOTU5ZWM0NGU3ZTdmIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBIYXlhdG8gS3Vyb2RhIDxrdXJvZGEuaGF5YXRvQGZ1aml0c3Uu
Y29tPgpEYXRlOiBXZWQsIDE5IEZlYiAyMDI1IDExOjM3OjI2ICswOTAwClN1YmplY3Q6IFtQQVRD
SCB2NF0gUHJvaGliaXQgc2xvdCBvcGVyYXRpb25zIHdoaWxlIGluIHRoZSBzaW5nbGUgdXNlciBt
b2RlCgotLS0KIHNyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvbG9naWNhbGZ1bmNzLmMg
fCAgMiArKwogc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xvdC5jICAgICAgICAgICAgICAgICB8
IDEyICsrKysrKysrKysrKwogc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNzLmMgICAg
ICAgICAgICB8IDEyICsrKysrKysrKysrKwogc3JjL2luY2x1ZGUvcmVwbGljYXRpb24vc2xvdC5o
ICAgICAgICAgICAgICAgICB8ICAxICsKIDQgZmlsZXMgY2hhbmdlZCwgMjcgaW5zZXJ0aW9ucygr
KQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvbG9naWNhbGZ1
bmNzLmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL2xvZ2ljYWxmdW5jcy5jCmlu
ZGV4IGNhNTNjYWFjMmYuLjE3OGEwN2FmN2QgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxp
Y2F0aW9uL2xvZ2ljYWwvbG9naWNhbGZ1bmNzLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRp
b24vbG9naWNhbC9sb2dpY2FsZnVuY3MuYwpAQCAtMTEzLDYgKzExMyw4IEBAIHBnX2xvZ2ljYWxf
c2xvdF9nZXRfY2hhbmdlc19ndXRzKEZ1bmN0aW9uQ2FsbEluZm8gZmNpbmZvLCBib29sIGNvbmZp
cm0sIGJvb2wgYmluCiAJTGlzdAkgICAqb3B0aW9ucyA9IE5JTDsKIAlEZWNvZGluZ091dHB1dFN0
YXRlICpwOwogCisJQ2hlY2tTbG90SXNJblNpbmdsZVVzZXJNb2RlKCk7CisKIAlDaGVja1Nsb3RQ
ZXJtaXNzaW9ucygpOwogCiAJQ2hlY2tMb2dpY2FsRGVjb2RpbmdSZXF1aXJlbWVudHMoKTsKZGlm
ZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3QuYyBiL3NyYy9iYWNrZW5kL3Jl
cGxpY2F0aW9uL3Nsb3QuYwppbmRleCA3MTllNTMxZWI5Li5kMDQxYWRmZTQ4IDEwMDY0NAotLS0g
YS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90LmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGlj
YXRpb24vc2xvdC5jCkBAIC0xNDQyLDYgKzE0NDIsMTggQEAgQ2hlY2tTbG90UGVybWlzc2lvbnMo
dm9pZCkKIAkJCQkJCSAgICJSRVBMSUNBVElPTiIpKSk7CiB9CiAKKy8qCisgKiBDaGVjayB3aGV0
aGVyIHRoZSBpbnN0YW5jZSBpcyBpbiBzaW5nbGUtdXNlciBtb2RlLgorICovCit2b2lkCitDaGVj
a1Nsb3RJc0luU2luZ2xlVXNlck1vZGUodm9pZCkKK3sKKwlpZiAoIUlzVW5kZXJQb3N0bWFzdGVy
KQorCQllcmVwb3J0KEVSUk9SLAorCQkJCShlcnJjb2RlKEVSUkNPREVfT0JKRUNUX05PVF9JTl9Q
UkVSRVFVSVNJVEVfU1RBVEUpLAorCQkJCSBlcnJtc2coInJlcGxpY2F0aW9uIHNsb3RzIGNhbm5v
dCBiZSB1c2VkIGluIHNpbmdsZS11c2VyIG1vZGUiKSkpOworfQorCiAvKgogICogUmVzZXJ2ZSBX
QUwgZm9yIHRoZSBjdXJyZW50bHkgYWN0aXZlIHNsb3QuCiAgKgpkaWZmIC0tZ2l0IGEvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNzLmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9z
bG90ZnVuY3MuYwppbmRleCAxNDZlZWY1ODcxLi5iNjY5Nzc0Yzg3IDEwMDY0NAotLS0gYS9zcmMv
YmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNh
dGlvbi9zbG90ZnVuY3MuYwpAQCAtMTcsNiArMTcsNyBAQAogI2luY2x1ZGUgImFjY2Vzcy94bG9n
cmVjb3ZlcnkuaCIKICNpbmNsdWRlICJhY2Nlc3MveGxvZ3V0aWxzLmgiCiAjaW5jbHVkZSAiZnVu
Y2FwaS5oIgorI2luY2x1ZGUgIm1pc2NhZG1pbi5oIgogI2luY2x1ZGUgInJlcGxpY2F0aW9uL2xv
Z2ljYWwuaCIKICNpbmNsdWRlICJyZXBsaWNhdGlvbi9zbG90LmgiCiAjaW5jbHVkZSAicmVwbGlj
YXRpb24vc2xvdHN5bmMuaCIKQEAgLTc2LDYgKzc3LDggQEAgcGdfY3JlYXRlX3BoeXNpY2FsX3Jl
cGxpY2F0aW9uX3Nsb3QoUEdfRlVOQ1RJT05fQVJHUykKIAlpZiAoZ2V0X2NhbGxfcmVzdWx0X3R5
cGUoZmNpbmZvLCBOVUxMLCAmdHVwZGVzYykgIT0gVFlQRUZVTkNfQ09NUE9TSVRFKQogCQllbG9n
KEVSUk9SLCAicmV0dXJuIHR5cGUgbXVzdCBiZSBhIHJvdyB0eXBlIik7CiAKKwlDaGVja1Nsb3RJ
c0luU2luZ2xlVXNlck1vZGUoKTsKKwogCUNoZWNrU2xvdFBlcm1pc3Npb25zKCk7CiAKIAlDaGVj
a1Nsb3RSZXF1aXJlbWVudHMoKTsKQEAgLTE4Miw2ICsxODUsOCBAQCBwZ19jcmVhdGVfbG9naWNh
bF9yZXBsaWNhdGlvbl9zbG90KFBHX0ZVTkNUSU9OX0FSR1MpCiAJaWYgKGdldF9jYWxsX3Jlc3Vs
dF90eXBlKGZjaW5mbywgTlVMTCwgJnR1cGRlc2MpICE9IFRZUEVGVU5DX0NPTVBPU0lURSkKIAkJ
ZWxvZyhFUlJPUiwgInJldHVybiB0eXBlIG11c3QgYmUgYSByb3cgdHlwZSIpOwogCisJQ2hlY2tT
bG90SXNJblNpbmdsZVVzZXJNb2RlKCk7CisKIAlDaGVja1Nsb3RQZXJtaXNzaW9ucygpOwogCiAJ
Q2hlY2tMb2dpY2FsRGVjb2RpbmdSZXF1aXJlbWVudHMoKTsKQEAgLTUxNSw2ICs1MjAsOCBAQCBw
Z19yZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UoUEdfRlVOQ1RJT05fQVJHUykKIAogCUFzc2VydCgh
TXlSZXBsaWNhdGlvblNsb3QpOwogCisJQ2hlY2tTbG90SXNJblNpbmdsZVVzZXJNb2RlKCk7CisK
IAlDaGVja1Nsb3RQZXJtaXNzaW9ucygpOwogCiAJaWYgKFhMb2dSZWNQdHJJc0ludmFsaWQobW92
ZXRvKSkKQEAgLTYxMiw5ICs2MTksMTIgQEAgY29weV9yZXBsaWNhdGlvbl9zbG90KEZ1bmN0aW9u
Q2FsbEluZm8gZmNpbmZvLCBib29sIGxvZ2ljYWxfc2xvdCkKIAlUdXBsZURlc2MJdHVwZGVzYzsK
IAlIZWFwVHVwbGUJdHVwbGU7CiAKKwogCWlmIChnZXRfY2FsbF9yZXN1bHRfdHlwZShmY2luZm8s
IE5VTEwsICZ0dXBkZXNjKSAhPSBUWVBFRlVOQ19DT01QT1NJVEUpCiAJCWVsb2coRVJST1IsICJy
ZXR1cm4gdHlwZSBtdXN0IGJlIGEgcm93IHR5cGUiKTsKIAorCUNoZWNrU2xvdElzSW5TaW5nbGVV
c2VyTW9kZSgpOworCiAJQ2hlY2tTbG90UGVybWlzc2lvbnMoKTsKIAogCWlmIChsb2dpY2FsX3Ns
b3QpCkBAIC04NzEsNiArODgxLDggQEAgcGdfc3luY19yZXBsaWNhdGlvbl9zbG90cyhQR19GVU5D
VElPTl9BUkdTKQogCWNoYXIJICAgKmVycjsKIAlTdHJpbmdJbmZvRGF0YSBhcHBfbmFtZTsKIAor
CUNoZWNrU2xvdElzSW5TaW5nbGVVc2VyTW9kZSgpOworCiAJQ2hlY2tTbG90UGVybWlzc2lvbnMo
KTsKIAogCWlmICghUmVjb3ZlcnlJblByb2dyZXNzKCkpCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVk
ZS9yZXBsaWNhdGlvbi9zbG90LmggYi9zcmMvaW5jbHVkZS9yZXBsaWNhdGlvbi9zbG90LmgKaW5k
ZXggZjVhMjRjY2ZiZi4uMjQ1MjNiMTAwMSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcmVwbGlj
YXRpb24vc2xvdC5oCisrKyBiL3NyYy9pbmNsdWRlL3JlcGxpY2F0aW9uL3Nsb3QuaApAQCAtMzA2
LDYgKzMwNiw3IEBAIGV4dGVybiB2b2lkIENoZWNrUG9pbnRSZXBsaWNhdGlvblNsb3RzKGJvb2wg
aXNfc2h1dGRvd24pOwogCiBleHRlcm4gdm9pZCBDaGVja1Nsb3RSZXF1aXJlbWVudHModm9pZCk7
CiBleHRlcm4gdm9pZCBDaGVja1Nsb3RQZXJtaXNzaW9ucyh2b2lkKTsKK2V4dGVybiB2b2lkIENo
ZWNrU2xvdElzSW5TaW5nbGVVc2VyTW9kZSh2b2lkKTsKIGV4dGVybiBSZXBsaWNhdGlvblNsb3RJ
bnZhbGlkYXRpb25DYXVzZQogCQkJR2V0U2xvdEludmFsaWRhdGlvbkNhdXNlKGNvbnN0IGNoYXIg
KmNhdXNlX25hbWUpOwogZXh0ZXJuIGNvbnN0IGNoYXIgKkdldFNsb3RJbnZhbGlkYXRpb25DYXVz
ZU5hbWUoUmVwbGljYXRpb25TbG90SW52YWxpZGF0aW9uQ2F1c2UgY2F1c2UpOwotLSAKMi40My41
Cgo=
--0000000000001b2f9c063ba109ce--