Connection limits/permissions, slotsync workers, etc
In connection with the discussion at [1]/messages/by-id/8befc845430ba1ae3748af900af298788e579c89.camel@cybertec.at, I started to look at
exactly which server processes ought to be subject to connection
limits (datconnlimit, ACL_CONNECT, and related checks). The
current situation seems to be an inconsistent mess.
Looking at InitPostgres, InitializeSessionUserId, and CheckMyDatabase,
we have several different permissions and resource-limiting checks
that may be enforced against an incoming process:
ReservedConnections/SuperuserReservedConnections
rolcanlogin
rolconnlimit
datallowconn
datconnlimit
database's ACL_CONNECT privilege
I want to argue that ReservedConnections, rolconnlimit, and
datconnlimit should only be applied to regular backends. It
makes no particular sense to enforce them against autovac workers,
background workers, or wal senders, because each of those types of
processes has its own resource-limiting PGPROC pool. It's especially
bad to enforce them against parallel workers, since that creates
edge-case failures that make the use of parallelism not transparent
to applications. We hacked around that at 73c9f91a1, but I think
that should be reverted in favor of not applying the check at all.
I further argue that rolcanlogin, datallowconn, and ACL_CONNECT
should not be checked in a parallel worker, again primarily on the
grounds that this creates parallelism-specific failures (cf [1]/messages/by-id/8befc845430ba1ae3748af900af298788e579c89.camel@cybertec.at).
The two scenarios where this occurs are (1) permissions were
revoked since the leader process connected, or (2) leader is
currently running as a role that wouldn't have permission to
connect on its own. We don't attempt to kick out the leader
process when either of those things happen, so why should we
prevent it from using parallelism?
The current situation for each of these checks is:
ReservedConnections is enforced if !am_superuser && !am_walsender,
so it is enforced against non-superuser background workers,
which is silly because BG workers have their own PGPROC pool;
moreover, what's the argument for letting walsenders but not other
kinds of background processes escape this? I propose changing it to
apply only to regular backends.
rolcanlogin is enforced if IsUnderPostmaster and we reach
InitializeSessionUserId, which basically reduces to regular backends,
parallel workers, logrep workers, and walsenders. Seems reasonable
for logrep workers and walsenders which represent fresh logins, but
not for parallel workers. I propose fixing this by making
ParallelWorkerMain pass BGWORKER_BYPASS_ROLELOGINCHECK.
rolconnlimit is enforced if IsUnderPostmaster and we reach
InitializeSessionUserId and it's not a superuser. So that applies
to non-superuser parallel workers, logrep workers, and walsenders,
and I don't think it's reasonable to apply it to any of them since
those all come out of other PGPROC pools. I propose switching that
to apply only to regular backends.
BTW, I kind of wonder why rolconnlimit is ineffectual for superusers,
especially when rolcanlogin does apply to them. Not a bug exactly,
but it sure seems inconsistent. If you've taken the trouble to set it
you'd expect it to work. Shall we take out the !is_superuser check?
datallowconn is enforced against all non-standalone, non-AV-worker
processes that connect to a specific database, except bgworkers that
pass BGWORKER_BYPASS_ALLOWCONN (nothing in core except test module).
So again that includes parallel workers, logrep workers, and
walsenders. Again this seems reasonable for logrep workers and
walsenders but not for parallel workers. I propose fixing this by
making ParallelWorkerMain pass BGWORKER_BYPASS_ALLOWCONN.
datconnlimit is enforced against all non-superuser processes,
including per-DB walsenders and BG workers (see above).
This is fairly dubious given that they have their own PGPROC pools.
I propose switching that to apply only to regular backends, too.
ACL_CONNECT is enforced against all non-superuser processes,
including per-DB walsenders and BG workers (includes
parallel workers, subscription apply workers, logrep workers).
Perhaps that's OK for most, but I argue not for parallel workers;
maybe skip it if BGWORKER_BYPASS_ALLOWCONN?
Also, the enforcement of datconnlimit and rolconnlimit is inconsistent
in another way: our counting of the pre-existing processes is pretty
random. CountDBConnections is not consistent with either the current
set of processes that datconnlimit is enforced against, or my proposal
to enforce it only against regular backends. It counts anything that
is not AmBackgroundWorkerProcess, including AV workers and per-DB
walsenders. I think it should count only regular backends, because
anything else leads to weird inconsistencies in whether a rejection
occurs.
The same applies to CountUserBackends (used for rolconnlimit check).
I argue these two functions should count only regular backends,
and the enforcement should likewise be only against regular backends.
Another recently-created problem is that the "slotsync worker"
process type we added in v17 hasn't been considered in any of this.
In particular, unlike every other process type that can obtain
a PGPROC, we did not consider it in the MaxBackends calculation:
/* the extra unit accounts for the autovacuum launcher */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
max_worker_processes + max_wal_senders;
This means AFAICS that it effectively competes for one of the
MaxConnections PGPROC slots, meaning that it could fail for lack of a
slot or could lock out a client that should have been able to connect.
Shouldn't we have added another dedicated PGPROC slot for it?
(proc.c would require some additional work to make that happen.)
I wonder if the AV launcher and slotsync worker could be reclassified
as "auxiliary processes" instead of being their own weird animal.
regards, tom lane
[1]: /messages/by-id/8befc845430ba1ae3748af900af298788e579c89.camel@cybertec.at
On Thursday, December 26, 2024 3:50 AM Tom Lane <tgl@sss.pgh.pa.us>
Hi,
In connection with the discussion at [1], I started to look at exactly which server
processes ought to be subject to connection limits (datconnlimit,
ACL_CONNECT, and related checks). The current situation seems to be an
inconsistent mess.Looking at InitPostgres, InitializeSessionUserId, and CheckMyDatabase, we
have several different permissions and resource-limiting checks that may be
enforced against an incoming process:
ReservedConnections/SuperuserReservedConnections
rolcanlogin
rolconnlimit
datallowconn
datconnlimit
database's ACL_CONNECT privilegeI want to argue that ReservedConnections, rolconnlimit, and datconnlimit
should only be applied to regular backends. It makes no particular sense to
enforce them against autovac workers, background workers, or wal senders,
because each of those types of processes has its own resource-limiting
PGPROC pool. It's especially bad to enforce them against parallel workers,
since that creates edge-case failures that make the use of parallelism not
transparent to applications. We hacked around that at 73c9f91a1, but I think
that should be reverted in favor of not applying the check at all.I further argue that rolcanlogin, datallowconn, and ACL_CONNECT should not
be checked in a parallel worker, again primarily on the grounds that this creates
parallelism-specific failures (cf [1]).
The two scenarios where this occurs are (1) permissions were revoked since
the leader process connected, or (2) leader is currently running as a role that
wouldn't have permission to connect on its own. We don't attempt to kick out
the leader process when either of those things happen, so why should we
prevent it from using parallelism?The current situation for each of these checks is:
ReservedConnections is enforced if !am_superuser && !am_walsender, so it is
enforced against non-superuser background workers, which is silly because
BG workers have their own PGPROC pool; moreover, what's the argument for
letting walsenders but not other kinds of background processes escape this?
I propose changing it to apply only to regular backends.rolcanlogin is enforced if IsUnderPostmaster and we reach
InitializeSessionUserId, which basically reduces to regular backends, parallel
workers, logrep workers, and walsenders. Seems reasonable for logrep
workers and walsenders which represent fresh logins, but not for parallel
workers. I propose fixing this by making ParallelWorkerMain pass
BGWORKER_BYPASS_ROLELOGINCHECK.rolconnlimit is enforced if IsUnderPostmaster and we reach
InitializeSessionUserId and it's not a superuser. So that applies to
non-superuser parallel workers, logrep workers, and walsenders, and I don't
think it's reasonable to apply it to any of them since those all come out of other
PGPROC pools. I propose switching that to apply only to regular backends.BTW, I kind of wonder why rolconnlimit is ineffectual for superusers, especially
when rolcanlogin does apply to them. Not a bug exactly, but it sure seems
inconsistent. If you've taken the trouble to set it you'd expect it to work. Shall
we take out the !is_superuser check?datallowconn is enforced against all non-standalone, non-AV-worker
processes that connect to a specific database, except bgworkers that pass
BGWORKER_BYPASS_ALLOWCONN (nothing in core except test module).
So again that includes parallel workers, logrep workers, and walsenders.
Again this seems reasonable for logrep workers and walsenders but not for
parallel workers. I propose fixing this by making ParallelWorkerMain pass
BGWORKER_BYPASS_ALLOWCONN.datconnlimit is enforced against all non-superuser processes, including
per-DB walsenders and BG workers (see above).
This is fairly dubious given that they have their own PGPROC pools.
I propose switching that to apply only to regular backends, too.ACL_CONNECT is enforced against all non-superuser processes, including
per-DB walsenders and BG workers (includes parallel workers, subscription
apply workers, logrep workers).
Perhaps that's OK for most, but I argue not for parallel workers; maybe skip it if
BGWORKER_BYPASS_ALLOWCONN?Also, the enforcement of datconnlimit and rolconnlimit is inconsistent in
another way: our counting of the pre-existing processes is pretty random.
CountDBConnections is not consistent with either the current set of processes
that datconnlimit is enforced against, or my proposal to enforce it only against
regular backends. It counts anything that is not
AmBackgroundWorkerProcess, including AV workers and per-DB walsenders.
I think it should count only regular backends, because anything else leads to
weird inconsistencies in whether a rejection occurs.The same applies to CountUserBackends (used for rolconnlimit check).
I argue these two functions should count only regular backends, and the
enforcement should likewise be only against regular backends.
Personally, I find these proposals to be reasonable.
Another recently-created problem is that the "slotsync worker"
process type we added in v17 hasn't been considered in any of this.
In particular, unlike every other process type that can obtain a PGPROC, we did
not consider it in the MaxBackends calculation:/* the extra unit accounts for the autovacuum launcher */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
max_worker_processes + max_wal_senders;This means AFAICS that it effectively competes for one of the MaxConnections
PGPROC slots, meaning that it could fail for lack of a slot or could lock out a
client that should have been able to connect.
Shouldn't we have added another dedicated PGPROC slot for it?
(proc.c would require some additional work to make that happen.)
Thanks for reporting the issue! I can reproduce the issue E.g., slotsync worker
cannot start when user backends have reached the max_connections limit. And I
agree that it would be better to add another dedicated PGPROC for it. I have
prepared a patch attached to address this.
I wonder if the AV launcher and slotsync worker could be reclassified as "auxiliary
processes" instead of being their own weird animal.
It appears that the current aux processes do not run transactions as stated in the
comments[1], so we may need to somehow release this restriction to achieve the
goal.
[1]
/messages/by-id/8befc845430ba1ae3748af900af2987
88e579c89.camel%40cybertec.at
Best Regards,
Hou zj
Attachments:
v1-0001-Reserve-a-dedicated-PGPROC-slot-for-slotsync-work.patchapplication/octet-stream; name=v1-0001-Reserve-a-dedicated-PGPROC-slot-for-slotsync-work.patchDownload
From 71a5f6bde59d6dd31f5915a089154d6acd09eea4 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Fri, 27 Dec 2024 14:25:51 +0800
Subject: [PATCH v1] Reserve a dedicated PGPROC slot for slotsync worker
Currently, a slot sync worker cannot start when user backends have reached
the max_connections limit. This occurs because the slot sync worker competes for
an entry within the max_connections allocation. To resolve this, it would be
better to provide a dedicated PGPROC slot for the slot sync worker, independent
of max_connections, similar to how autovacuum launcher/workers and WAL sender
processes are handled. This patch implements this solution to resolve the
issue.
---
src/backend/storage/lmgr/proc.c | 19 ++++++++++++++-----
src/backend/utils/init/postinit.c | 7 +++++--
src/include/storage/proc.h | 8 ++++++++
3 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 720ef99ee8..9cb1229286 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -189,6 +189,7 @@ InitProcGlobal(void)
dlist_init(&ProcGlobal->autovacFreeProcs);
dlist_init(&ProcGlobal->bgworkerFreeProcs);
dlist_init(&ProcGlobal->walsenderFreeProcs);
+ dlist_init(&ProcGlobal->slotsyncFreeProcs);
ProcGlobal->startupBufferPinWaitBufId = -1;
ProcGlobal->walwriterProc = INVALID_PROC_NUMBER;
ProcGlobal->checkpointerProc = INVALID_PROC_NUMBER;
@@ -198,10 +199,10 @@ InitProcGlobal(void)
/*
* Create and initialize all the PGPROC structures we'll need. There are
* five separate consumers: (1) normal backends, (2) autovacuum workers
- * and the autovacuum launcher, (3) background workers, (4) auxiliary
- * processes, and (5) prepared transactions. Each PGPROC structure is
- * dedicated to exactly one of these purposes, and they do not move
- * between groups.
+ * and the autovacuum launcher, (3) background workers, (4) the slot sync
+ * worker, (5) auxiliary processes, and (6) prepared transactions. Each
+ * PGPROC structure is dedicated to exactly one of these purposes, and
+ * they do not move between groups.
*/
procs = (PGPROC *) ShmemAlloc(TotalProcs * sizeof(PGPROC));
MemSet(procs, 0, TotalProcs * sizeof(PGPROC));
@@ -294,12 +295,18 @@ InitProcGlobal(void)
dlist_push_tail(&ProcGlobal->bgworkerFreeProcs, &proc->links);
proc->procgloballist = &ProcGlobal->bgworkerFreeProcs;
}
- else if (i < MaxBackends)
+ else if (i < MaxConnections + autovacuum_max_workers + 1 + max_worker_processes + max_wal_senders)
{
/* PGPROC for walsender, add to walsenderFreeProcs list */
dlist_push_tail(&ProcGlobal->walsenderFreeProcs, &proc->links);
proc->procgloballist = &ProcGlobal->walsenderFreeProcs;
}
+ else if (i < MaxBackends)
+ {
+ /* PGPROC for slot sync worker, add to slotsyncFreeProcs list */
+ dlist_push_tail(&ProcGlobal->slotsyncFreeProcs, &proc->links);
+ proc->procgloballist = &ProcGlobal->slotsyncFreeProcs;
+ }
/* Initialize myProcLocks[] shared memory queues. */
for (j = 0; j < NUM_LOCK_PARTITIONS; j++)
@@ -365,6 +372,8 @@ InitProcess(void)
procgloballist = &ProcGlobal->bgworkerFreeProcs;
else if (AmWalSenderProcess())
procgloballist = &ProcGlobal->walsenderFreeProcs;
+ else if (AmLogicalSlotSyncWorkerProcess())
+ procgloballist = &ProcGlobal->slotsyncFreeProcs;
else
procgloballist = &ProcGlobal->freeProcs;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 5b657a3f13..2c2f06e597 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -544,9 +544,12 @@ InitializeMaxBackends(void)
{
Assert(MaxBackends == 0);
- /* the extra unit accounts for the autovacuum launcher */
+ /*
+ * The extra units account for the autovacuum launcher and the slot sync
+ * worker.
+ */
MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- max_worker_processes + max_wal_senders;
+ max_worker_processes + max_wal_senders + 1;
if (MaxBackends > MAX_BACKENDS)
ereport(ERROR,
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 5a3dd5d2d4..d0b197e148 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -414,6 +414,14 @@ typedef struct PROC_HDR
dlist_head bgworkerFreeProcs;
/* Head of list of walsender free PGPROC structures */
dlist_head walsenderFreeProcs;
+
+ /*
+ * Head of list of slot sync worker free PGPROC structures. Only one slot
+ * sync worker is supported for now, but storing it in a list format to be
+ * consistent and to make it easier to support multiple workers in the
+ * future.
+ */
+ dlist_head slotsyncFreeProcs;
/* First pgproc waiting for group XID clear */
pg_atomic_uint32 procArrayGroupFirst;
/* First pgproc waiting for group transaction status update */
--
2.31.1
"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com> writes:
On Thursday, December 26, 2024 3:50 AM Tom Lane <tgl@sss.pgh.pa.us>
I wonder if the AV launcher and slotsync worker could be reclassified as "auxiliary
processes" instead of being their own weird animal.
It appears that the current aux processes do not run transactions as stated in the
comments[1], so we may need to somehow release this restriction to achieve the
goal.
Ah, right, I'd forgotten about that restriction. I agree that
removing it wouldn't be very reasonable. However, I still would
rather avoid making the slotsync worker be its very own special
snowflake, because that offers no support for the next person
who wants to invent a new sort of specialized transaction-capable
process.
Attached is an alternative proposal that groups the autovac launcher
and slotsync worker into a new category of "special workers" (better
name welcome). I chose to put them into the existing autovacFreeProcs
freelist, partly because the autovac launcher lives there already
but mostly because I don't want to add another freelist in a patch
we need to put into v17. (As written, your patch is an ABI break.
It'd probably be safe to add a new freelist at the end of the struct
in v17, but I'm a little shy about that in view of recent bugs. In
any case, a freelist having at most two members seems rather silly.)
I was amused but not terribly surprised to notice that the comments
in InitProcGlobal were *already* out of date, in that they didn't
account for the walsender PGPROC pool. We have a remarkably bad
track record for updating comments that are more than about two
lines away from the code they describe :-(
Thoughts?
regards, tom lane
Attachments:
v2-reserve-a-PGPROC-for-slotsync-worker.patchtext/x-diff; charset=us-ascii; name=v2-reserve-a-PGPROC-for-slotsync-worker.patchDownload
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 720ef99ee8..10d4fb4ea1 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -197,11 +197,12 @@ InitProcGlobal(void)
/*
* Create and initialize all the PGPROC structures we'll need. There are
- * five separate consumers: (1) normal backends, (2) autovacuum workers
- * and the autovacuum launcher, (3) background workers, (4) auxiliary
- * processes, and (5) prepared transactions. Each PGPROC structure is
- * dedicated to exactly one of these purposes, and they do not move
- * between groups.
+ * six separate consumers: (1) normal backends, (2) autovacuum workers and
+ * special workers, (3) background workers, (4) walsenders, (5) auxiliary
+ * processes, and (6) prepared transactions. (For largely-historical
+ * reasons, we combine autovacuum and special workers into one category
+ * with a single freelist.) Each PGPROC structure is dedicated to exactly
+ * one of these purposes, and they do not move between groups.
*/
procs = (PGPROC *) ShmemAlloc(TotalProcs * sizeof(PGPROC));
MemSet(procs, 0, TotalProcs * sizeof(PGPROC));
@@ -269,12 +270,13 @@ InitProcGlobal(void)
}
/*
- * Newly created PGPROCs for normal backends, autovacuum and bgworkers
- * must be queued up on the appropriate free list. Because there can
- * only ever be a small, fixed number of auxiliary processes, no free
- * list is used in that case; InitAuxiliaryProcess() instead uses a
- * linear search. PGPROCs for prepared transactions are added to a
- * free list by TwoPhaseShmemInit().
+ * Newly created PGPROCs for normal backends, autovacuum workers,
+ * special workers, bgworkers, and walsenders must be queued up on the
+ * appropriate free list. Because there can only ever be a small,
+ * fixed number of auxiliary processes, no free list is used in that
+ * case; InitAuxiliaryProcess() instead uses a linear search. PGPROCs
+ * for prepared transactions are added to a free list by
+ * TwoPhaseShmemInit().
*/
if (i < MaxConnections)
{
@@ -282,13 +284,13 @@ InitProcGlobal(void)
dlist_push_tail(&ProcGlobal->freeProcs, &proc->links);
proc->procgloballist = &ProcGlobal->freeProcs;
}
- else if (i < MaxConnections + autovacuum_max_workers + 1)
+ else if (i < MaxConnections + autovacuum_max_workers + NUM_SPECIAL_WORKER_PROCS)
{
- /* PGPROC for AV launcher/worker, add to autovacFreeProcs list */
+ /* PGPROC for AV or special worker, add to autovacFreeProcs list */
dlist_push_tail(&ProcGlobal->autovacFreeProcs, &proc->links);
proc->procgloballist = &ProcGlobal->autovacFreeProcs;
}
- else if (i < MaxConnections + autovacuum_max_workers + 1 + max_worker_processes)
+ else if (i < MaxConnections + autovacuum_max_workers + NUM_SPECIAL_WORKER_PROCS + max_worker_processes)
{
/* PGPROC for bgworker, add to bgworkerFreeProcs list */
dlist_push_tail(&ProcGlobal->bgworkerFreeProcs, &proc->links);
@@ -358,8 +360,11 @@ InitProcess(void)
if (IsUnderPostmaster)
RegisterPostmasterChildActive();
- /* Decide which list should supply our PGPROC. */
- if (AmAutoVacuumLauncherProcess() || AmAutoVacuumWorkerProcess())
+ /*
+ * Decide which list should supply our PGPROC. This logic must match the
+ * way the freelists were constructed in InitProcGlobal().
+ */
+ if (AmAutoVacuumWorkerProcess() || AmSpecialWorkerProcess())
procgloballist = &ProcGlobal->autovacFreeProcs;
else if (AmBackgroundWorkerProcess())
procgloballist = &ProcGlobal->bgworkerFreeProcs;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 770ab6906e..9731de5781 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -544,9 +544,9 @@ InitializeMaxBackends(void)
{
Assert(MaxBackends == 0);
- /* the extra unit accounts for the autovacuum launcher */
- MaxBackends = MaxConnections + autovacuum_max_workers + 1 +
- max_worker_processes + max_wal_senders;
+ /* Note that this does not include "auxiliary" processes */
+ MaxBackends = MaxConnections + autovacuum_max_workers +
+ max_worker_processes + max_wal_senders + NUM_SPECIAL_WORKER_PROCS;
if (MaxBackends > MAX_BACKENDS)
ereport(ERROR,
@@ -555,7 +555,7 @@ InitializeMaxBackends(void)
errdetail("\"max_connections\" (%d) plus \"autovacuum_max_workers\" (%d) plus \"max_worker_processes\" (%d) plus \"max_wal_senders\" (%d) must be less than %d.",
MaxConnections, autovacuum_max_workers,
max_worker_processes, max_wal_senders,
- MAX_BACKENDS)));
+ MAX_BACKENDS - (NUM_SPECIAL_WORKER_PROCS - 1))));
}
/*
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 3f97fcef80..d07181f932 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -350,8 +350,9 @@ typedef enum BackendType
/*
* Auxiliary processes. These have PGPROC entries, but they are not
- * attached to any particular database. There can be only one of each of
- * these running at a time.
+ * attached to any particular database, and cannot run transactions or
+ * even take heavyweight locks. There can be only one of each of these
+ * running at a time.
*
* If you modify these, make sure to update NUM_AUXILIARY_PROCS and the
* glossary in the docs.
@@ -388,6 +389,10 @@ extern PGDLLIMPORT BackendType MyBackendType;
#define AmWalSummarizerProcess() (MyBackendType == B_WAL_SUMMARIZER)
#define AmWalWriterProcess() (MyBackendType == B_WAL_WRITER)
+#define AmSpecialWorkerProcess() \
+ (AmAutoVacuumLauncherProcess() || \
+ AmLogicalSlotSyncWorkerProcess())
+
extern const char *GetBackendTypeDesc(BackendType backendType);
extern void SetDatabasePath(const char *path);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 5a3dd5d2d4..14e34d4e93 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -408,7 +408,7 @@ typedef struct PROC_HDR
uint32 allProcCount;
/* Head of list of free PGPROC structures */
dlist_head freeProcs;
- /* Head of list of autovacuum's free PGPROC structures */
+ /* Head of list of autovacuum & special worker free PGPROC structures */
dlist_head autovacFreeProcs;
/* Head of list of bgworker free PGPROC structures */
dlist_head bgworkerFreeProcs;
@@ -442,9 +442,19 @@ extern PGDLLIMPORT PGPROC *PreparedXactProcs;
#define GetPGProcByNumber(n) (&ProcGlobal->allProcs[(n)])
#define GetNumberFromPGProc(proc) ((proc) - &ProcGlobal->allProcs[0])
+/*
+ * We set aside some extra PGPROC structures for "special worker" processes,
+ * which are full-fledged backends (they can run transactions)
+ * but are unique animals that there's never more than one of.
+ * Currently there are two such processes: the autovacuum launcher
+ * and the slotsync worker.
+ */
+#define NUM_SPECIAL_WORKER_PROCS 2
+
/*
* We set aside some extra PGPROC structures for auxiliary processes,
- * ie things that aren't full-fledged backends but need shmem access.
+ * ie things that aren't full-fledged backends (they cannot run transactions
+ * or take heavyweight locks) but need shmem access.
*
* Background writer, checkpointer, WAL writer, WAL summarizer, and archiver
* run during normal operation. Startup process and WAL receiver also consume
Also, here's a patch for the rest of what I was talking about.
We'll need to back-patch this given that the CVE-2024-10978 changes
caused these sorts of problems in all branches, but I've not yet
attempted to back-patch. It looks like it might be a bit painful
thanks to past code churn in these areas.
I didn't do anything about the idea of making rolconnlimit applicable
to superusers. If we do that at all, it should only be in HEAD.
Also, I got a shade less enthusiastic about it after noting that this
logic is parallel to that for datconnlimit, and it does seems sensible
to allow superusers to ignore datconnlimit. Maybe it's fine for the
two limits to operate differently, but I'm unsure.
Also, it probably would make sense to rename PGPROC.isBackgroundWorker
to isRegularBackend (inverting the sense of the boolean), but that
doesn't seem like back-patch material either, so I didn't include it
here. I think we can get away with a subtle adjustment of which
processes that flag is set for in the back branches, but not with
renaming it.
regards, tom lane
Attachments:
v1-0001-Exclude-parallel-workers-from-connection-privileg.patchtext/x-diff; charset=us-ascii; name*0=v1-0001-Exclude-parallel-workers-from-connection-privileg.p; name*1=atchDownload
From 84017bf4da912cad44416b729860baf2edfe66fd Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 27 Dec 2024 15:36:31 -0500
Subject: [PATCH v1] Exclude parallel workers from connection privilege/limit
checks.
Cause parallel workers to not check datallowconn, rolcanlogin, and
ACL_CONNECT privileges. The leader already checked these things
(except for rolcanlogin which might have been checked for a different
role). Re-checking can accomplish little except to induce unexpected
failures in applications that might not even be aware that their query
has been parallelized. We already had the principle that parallel
workers rely on their leader to pass a valid set of authorization
information, so this change just extends that a bit further.
Also, modify the ReservedConnections, datconnlimit and rolconnlimit
logic so that these limits are only enforced against regular backends,
and only regular backends are counted while checking if the limits
were already reached. Previously, background processes that had an
assigned database or role were subject to these limits (with rather
random exclusions for autovac workers and walsenders), and the set of
existing processes that counted against each limit was quite haphazard
as well. The point of these limits, AFAICS, is to ensure the
availability of PGPROC slots for regular backends. Since all other
types of processes have their own separate pools of PGPROC slots, it
makes no sense either to enforce these limits against them or to count
them while enforcing the limit.
While edge-case failures of these sorts have been possible for a
long time, the problem got a good deal worse with commit 5a2fed911
(CVE-2024-10978), which caused parallel workers to make these checks
using the leader's current role instead of its AuthenticatedUserId,
thus allowing parallel queries to fail after SET ROLE. That older
behavior was fairly accidental and I have no desire to return to it.
This patch allows reverting 73c9f91a1 which was an emergency hack
to suppress these same checks in some cases. It wasn't complete,
as shown by a recent bug report from Laurenz Albe. We can also
revert fd4d93d26 and 492217301, which hacked around the problems in
one regression test.
Like 5a2fed911, back-patch to supported branches (which sadly no
longer includes v12).
Discussion: https://postgr.es/m/1808397.1735156190@sss.pgh.pa.us
---
src/backend/access/transam/parallel.c | 10 ++++--
src/backend/access/transam/twophase.c | 2 +-
src/backend/postmaster/bgworker.c | 4 +--
src/backend/storage/ipc/procarray.c | 4 +--
src/backend/storage/lmgr/proc.c | 4 +--
src/backend/utils/init/miscinit.c | 8 ++++-
src/backend/utils/init/postinit.c | 40 ++++++++----------------
src/include/miscadmin.h | 1 +
src/include/storage/proc.h | 2 +-
src/test/modules/worker_spi/worker_spi.c | 10 ------
10 files changed, 37 insertions(+), 48 deletions(-)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 0a1e089ec1..60d95037b3 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1419,10 +1419,16 @@ ParallelWorkerMain(Datum main_arg)
fps->session_user_is_superuser);
SetCurrentRoleId(fps->outer_user_id, fps->role_is_superuser);
- /* Restore database connection. */
+ /*
+ * Restore database connection. We skip connection authorization checks,
+ * reasoning that (a) the leader checked these things when it started, and
+ * (b) we do not want parallel mode to cause these failures, because that
+ * would make use of parallel query plans not transparent to applications.
+ */
BackgroundWorkerInitializeConnectionByOid(fps->database_id,
fps->authenticated_user_id,
- 0);
+ BGWORKER_BYPASS_ALLOWCONN |
+ BGWORKER_BYPASS_ROLELOGINCHECK);
/*
* Set the client encoding to the database encoding, since that is what
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 49be1df91c..edb8e06081 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -466,7 +466,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid,
proc->databaseId = databaseid;
proc->roleId = owner;
proc->tempNamespaceId = InvalidOid;
- proc->isBackgroundWorker = false;
+ proc->isBackgroundWorker = true;
proc->lwWaiting = LW_WS_NOT_WAITING;
proc->lwWaitMode = 0;
proc->waitLock = NULL;
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index 07bc5517fc..7afe56885c 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -854,7 +854,7 @@ BackgroundWorkerInitializeConnection(const char *dbname, const char *username, u
BackgroundWorker *worker = MyBgworkerEntry;
bits32 init_flags = 0; /* never honor session_preload_libraries */
- /* ignore datallowconn? */
+ /* ignore datallowconn and ACL_CONNECT? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
/* ignore rolcanlogin? */
@@ -888,7 +888,7 @@ BackgroundWorkerInitializeConnectionByOid(Oid dboid, Oid useroid, uint32 flags)
BackgroundWorker *worker = MyBgworkerEntry;
bits32 init_flags = 0; /* never honor session_preload_libraries */
- /* ignore datallowconn? */
+ /* ignore datallowconn and ACL_CONNECT? */
if (flags & BGWORKER_BYPASS_ALLOWCONN)
init_flags |= INIT_PG_OVERRIDE_ALLOW_CONNS;
/* ignore rolcanlogin? */
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index c769b1aa3e..a640b6f5f7 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3622,8 +3622,7 @@ CountDBBackends(Oid databaseid)
}
/*
- * CountDBConnections --- counts database backends ignoring any background
- * worker processes
+ * CountDBConnections --- counts database backends (only regular backends)
*/
int
CountDBConnections(Oid databaseid)
@@ -3695,6 +3694,7 @@ CancelDBBackends(Oid databaseid, ProcSignalReason sigmode, bool conflictPending)
/*
* CountUserBackends --- count backends that are used by specified user
+ * (only regular backends, not any type of background worker)
*/
int
CountUserBackends(Oid roleid)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 720ef99ee8..9bce100256 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -427,7 +427,7 @@ InitProcess(void)
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
- MyProc->isBackgroundWorker = AmBackgroundWorkerProcess();
+ MyProc->isBackgroundWorker = !AmRegularBackendProcess();
MyProc->delayChkptFlags = 0;
MyProc->statusFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
@@ -626,7 +626,7 @@ InitAuxiliaryProcess(void)
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
MyProc->tempNamespaceId = InvalidOid;
- MyProc->isBackgroundWorker = AmBackgroundWorkerProcess();
+ MyProc->isBackgroundWorker = true;
MyProc->delayChkptFlags = 0;
MyProc->statusFlags = 0;
MyProc->lwWaiting = LW_WS_NOT_WAITING;
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index d24ac133fb..68a917c8fb 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -844,6 +844,9 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
* These next checks are not enforced when in standalone mode, so that
* there is a way to recover from sillinesses like "UPDATE pg_authid SET
* rolcanlogin = false;".
+ *
+ * We also do not enforce rolcanlogin in background workers that set
+ * bypass_login_check.
*/
if (IsUnderPostmaster)
{
@@ -857,7 +860,9 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
rname)));
/*
- * Check connection limit for this role.
+ * Check connection limit for this role. We enforce the limit only
+ * for regular backends, since other process types have their own
+ * PGPROC pools.
*
* There is a race condition here --- we create our PGPROC before
* checking for other PGPROCs. If two backends did this at about the
@@ -867,6 +872,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
* just document that the connection limit is approximate.
*/
if (rform->rolconnlimit >= 0 &&
+ AmRegularBackendProcess() &&
!is_superuser &&
CountUserBackends(roleid) > rform->rolconnlimit)
ereport(FATAL,
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 770ab6906e..af3b030741 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -22,7 +22,6 @@
#include "access/genam.h"
#include "access/heapam.h"
#include "access/htup_details.h"
-#include "access/parallel.h"
#include "access/session.h"
#include "access/tableam.h"
#include "access/xact.h"
@@ -342,7 +341,8 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
* a way to recover from disabling all access to all databases, for
* example "UPDATE pg_database SET datallowconn = false;".
*
- * We do not enforce them for autovacuum worker processes either.
+ * We do not enforce them for autovacuum worker processes either, nor for
+ * background workers that set override_allow_connections.
*/
if (IsUnderPostmaster && !AmAutoVacuumWorkerProcess())
{
@@ -360,7 +360,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
* is redundant, but since we have the flag, might as well check it
* and save a few cycles.)
*/
- if (!am_superuser &&
+ if (!am_superuser && !override_allow_connections &&
object_aclcheck(DatabaseRelationId, MyDatabaseId, GetUserId(),
ACL_CONNECT) != ACLCHECK_OK)
ereport(FATAL,
@@ -369,7 +369,9 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
errdetail("User does not have CONNECT privilege.")));
/*
- * Check connection limit for this database.
+ * Check connection limit for this database. We enforce the limit
+ * only for regular backends, since other process types have their own
+ * PGPROC pools.
*
* There is a race condition here --- we create our PGPROC before
* checking for other PGPROCs. If two backends did this at about the
@@ -379,6 +381,7 @@ CheckMyDatabase(const char *name, bool am_superuser, bool override_allow_connect
* just document that the connection limit is approximate.
*/
if (dbform->datconnlimit >= 0 &&
+ AmRegularBackendProcess() &&
!am_superuser &&
CountDBConnections(MyDatabaseId) > dbform->datconnlimit)
ereport(FATAL,
@@ -865,23 +868,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
{
InitializeSessionUserId(username, useroid,
(flags & INIT_PG_OVERRIDE_ROLE_LOGIN) != 0);
-
- /*
- * In a parallel worker, set am_superuser based on the
- * authenticated user ID, not the current role. This is pretty
- * dubious but it matches our historical behavior. Note that this
- * value of am_superuser is used only for connection-privilege
- * checks here and in CheckMyDatabase (we won't reach
- * process_startup_options in a background worker).
- *
- * In other cases, there's been no opportunity for the current
- * role to diverge from the authenticated user ID yet, so we can
- * just rely on superuser() and avoid an extra catalog lookup.
- */
- if (InitializingParallelWorker)
- am_superuser = superuser_arg(GetAuthenticatedUserId());
- else
- am_superuser = superuser();
+ am_superuser = superuser();
}
}
else
@@ -908,17 +895,16 @@ InitPostgres(const char *in_dbname, Oid dboid,
}
/*
- * The last few connection slots are reserved for superusers and roles
- * with privileges of pg_use_reserved_connections. Replication
- * connections are drawn from slots reserved with max_wal_senders and are
- * not limited by max_connections, superuser_reserved_connections, or
- * reserved_connections.
+ * The last few regular connection slots are reserved for superusers and
+ * roles with privileges of pg_use_reserved_connections. We do not apply
+ * these limits to background processes, since they all have their own
+ * pools of PGPROC slots.
*
* Note: At this point, the new backend has already claimed a proc struct,
* so we must check whether the number of free slots is strictly less than
* the reserved connection limits.
*/
- if (!am_superuser && !am_walsender &&
+ if (AmRegularBackendProcess() && !am_superuser &&
(SuperuserReservedConnections + ReservedConnections) > 0 &&
!HaveNFreeProcs(SuperuserReservedConnections + ReservedConnections, &nfree))
{
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 3f97fcef80..4e3f5fbbca 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -375,6 +375,7 @@ typedef enum BackendType
extern PGDLLIMPORT BackendType MyBackendType;
+#define AmRegularBackendProcess() (MyBackendType == B_BACKEND)
#define AmAutoVacuumLauncherProcess() (MyBackendType == B_AUTOVAC_LAUNCHER)
#define AmAutoVacuumWorkerProcess() (MyBackendType == B_AUTOVAC_WORKER)
#define AmBackgroundWorkerProcess() (MyBackendType == B_BG_WORKER)
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 5a3dd5d2d4..9dba393da3 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -216,7 +216,7 @@ struct PGPROC
Oid tempNamespaceId; /* OID of temp schema this backend is
* using */
- bool isBackgroundWorker; /* true if background worker. */
+ bool isBackgroundWorker; /* true if not a regular backend. */
/*
* While in hot standby mode, shows that a conflict signal has been sent
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index d4403b24d9..cf5b7505ec 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -169,16 +169,6 @@ worker_spi_main(Datum main_arg)
BackgroundWorkerInitializeConnection(worker_spi_database,
worker_spi_role, flags);
- /*
- * Disable parallel query for workers started with
- * BGWORKER_BYPASS_ALLOWCONN or BGWORKER_BYPASS_ROLELOGINCHECK so as these
- * don't attempt connections using a database or a role that may not allow
- * that.
- */
- if ((flags & (BGWORKER_BYPASS_ALLOWCONN | BGWORKER_BYPASS_ROLELOGINCHECK)))
- SetConfigOption("max_parallel_workers_per_gather", "0",
- PGC_USERSET, PGC_S_OVERRIDE);
-
elog(LOG, "%s initialized with %s.%s",
MyBgworkerEntry->bgw_name, table->schema, table->name);
initialize_worker_spi(table);
--
2.43.5
On Saturday, December 28, 2024 1:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com> writes:
On Thursday, December 26, 2024 3:50 AM Tom Lane <tgl@sss.pgh.pa.us>
I wonder if the AV launcher and slotsync worker could be reclassified
as "auxiliary processes" instead of being their own weird animal.It appears that the current aux processes do not run transactions as
stated in the comments[1], so we may need to somehow release this
restriction to achieve the goal.Ah, right, I'd forgotten about that restriction. I agree that removing it wouldn't
be very reasonable. However, I still would rather avoid making the slotsync
worker be its very own special snowflake, because that offers no support for
the next person who wants to invent a new sort of specialized
transaction-capable process.Attached is an alternative proposal that groups the autovac launcher and
slotsync worker into a new category of "special workers" (better name
welcome). I chose to put them into the existing autovacFreeProcs freelist,
partly because the autovac launcher lives there already but mostly because I
don't want to add another freelist in a patch we need to put into v17. (As
written, your patch is an ABI break.
Right, thanks for pointing it out. The new version patch looks good to me.
Best Regards,
Hou zj
"Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com> writes:
On Saturday, December 28, 2024 1:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Attached is an alternative proposal that groups the autovac launcher and
slotsync worker into a new category of "special workers" (better name
welcome). I chose to put them into the existing autovacFreeProcs freelist,
partly because the autovac launcher lives there already but mostly because I
don't want to add another freelist in a patch we need to put into v17. (As
written, your patch is an ABI break.
Right, thanks for pointing it out. The new version patch looks good to me.
Pushed, thanks for looking at it.
regards, tom lane