public inbox for [email protected]
help / color / mirror / Atom feedRe: Periodic authorization expiration checks using GoAway message
5+ messages / 3 participants
[nested] [flat]
* Re: Periodic authorization expiration checks using GoAway message
@ 2026-01-20 09:28 Zsolt Parragi <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Zsolt Parragi @ 2026-01-20 09:28 UTC (permalink / raw)
To: Ajit Awekar <[email protected]>; +Cc: Jacob Champion <[email protected]>; Jelte Fennema-Nio <[email protected]>; Hannu Krosing <[email protected]>; pgsql-hackers; Dave Cramer <[email protected]>; Heikki Linnakangas <[email protected]>
Hello!
I noticed a few things in the patch, please consider the following:
postgres.c:1076: elsewhere password_valid_until_timestamp is set to 0
when NULL, won't that result in unintended disconnection for users?
postgres.c:99: it only checks the expiration in exec_simple_query,
shouldn't it also be part of other methods (like
exec_execute_message)?
postgres.c:179: isn't the sys_cache_register_callback variable name a
bit too generic, shouldn't it have a more specific name related to
password expiration / authentication?
postgres.c:1082: the errhint text should have a period at the end.
postgres.c:4185: The comment for CheckPasswordExpiration says that the
function terminates the connection with FATAL, but the termination is
actually at the call site at line 1077. Maybe it would be better to
move that if/error inside the function, as the comment explains?
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Periodic authorization expiration checks using GoAway message
@ 2026-01-20 12:31 Ajit Awekar <[email protected]>
parent: Zsolt Parragi <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Ajit Awekar @ 2026-01-20 12:31 UTC (permalink / raw)
To: Zsolt Parragi <[email protected]>; +Cc: Jacob Champion <[email protected]>; Jelte Fennema-Nio <[email protected]>; Hannu Krosing <[email protected]>; pgsql-hackers; Dave Cramer <[email protected]>; Heikki Linnakangas <[email protected]>
Hi Zsolt,
Thanks a lot for your review comments.
>postgres.c:1076: elsewhere password_valid_until_timestamp is set to 0
>when NULL, won't that result in unintended disconnection for users?
Done. I have modified the condition check so as it will not impact users
having rolvaliduntil to NULL.
>postgres.c:99: it only checks the expiration in exec_simple_query,
>shouldn't it also be part of other methods (like
>exec_execute_message)?
I missed it. In the attached patch validation is now performed across all
primary query execution entry points to cover both simple and Extended
query protocols.
>postgres.c:179: isn't the sys_cache_register_callback variable name a
>bit too generic, shouldn't it have a more specific name related to
>password expiration / authentication?
I agree. I have renamed the variable to a more appropriate name
password_auth_cache_callback_registered.
> postgres.c:1082: the errhint text should have a period at the end.
Done.
>postgres.c:4185: The comment for CheckPasswordExpiration says that the
>function terminates the connection with FATAL, but the termination is
>actually at the call site at line 1077. Maybe it would be better to
>move that if/error inside the function, as the comment explains?
Done.
I have attached an updated patch. Request a review.
Thanks & Best Regards,
Ajit
On Tue, 20 Jan 2026 at 14:59, Zsolt Parragi <[email protected]>
wrote:
> Hello!
>
> I noticed a few things in the patch, please consider the following:
>
> postgres.c:1076: elsewhere password_valid_until_timestamp is set to 0
> when NULL, won't that result in unintended disconnection for users?
>
> postgres.c:99: it only checks the expiration in exec_simple_query,
> shouldn't it also be part of other methods (like
> exec_execute_message)?
>
> postgres.c:179: isn't the sys_cache_register_callback variable name a
> bit too generic, shouldn't it have a more specific name related to
> password expiration / authentication?
>
> postgres.c:1082: the errhint text should have a period at the end.
>
> postgres.c:4185: The comment for CheckPasswordExpiration says that the
> function terminates the connection with FATAL, but the termination is
> actually at the call site at line 1077. Maybe it would be better to
> move that if/error inside the function, as the comment explains?
>
Attachments:
[text/x-patch] password_expiration_enforcement_V1.diff (9.9K, ../../CAER375OZojMNP+_Gs4PeatbmbkfOfJ=fxJJ0U15HChnQJfLuUA@mail.gmail.com/3-password_expiration_enforcement_V1.diff)
download | inline diff:
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index 4c1052b3d42..eafc4309748 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -20,6 +20,7 @@
#include "common/scram-common.h"
#include "libpq/crypt.h"
#include "libpq/scram.h"
+#include "miscadmin.h"
#include "utils/builtins.h"
#include "utils/syscache.h"
#include "utils/timestamp.h"
@@ -66,7 +67,19 @@ get_role_password(const char *role, const char **logdetail)
datum = SysCacheGetAttr(AUTHNAME, roleTup,
Anum_pg_authid_rolvaliduntil, &isnull);
if (!isnull)
+ {
vuntil = DatumGetTimestampTz(datum);
+ /*
+ * Cache the password expiration timestamp from pg_authid.rolvaliduntil
+ * during initial authentication so it can be checked throughout the
+ * lifetime of the connection. By changing this value from -1 to >= 0
+ * we signal that password authentication was used.
+ */
+ password_valid_until_timestamp = vuntil;
+ }
+ /* No expiration limit set */
+ else
+ password_valid_until_timestamp = 0;
ReleaseSysCache(roleTup);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e54bf1e760f..6a62cfc50bd 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -34,6 +34,7 @@
#include "access/parallel.h"
#include "access/printtup.h"
#include "access/xact.h"
+#include "catalog/pg_authid.h"
#include "catalog/pg_type.h"
#include "commands/async.h"
#include "commands/event_trigger.h"
@@ -74,10 +75,12 @@
#include "tcop/utility.h"
#include "utils/guc_hooks.h"
#include "utils/injection_point.h"
+#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/snapmgr.h"
+#include "utils/syscache.h"
#include "utils/timeout.h"
#include "utils/timestamp.h"
#include "utils/varlena.h"
@@ -105,6 +108,13 @@ int client_connection_check_interval = 0;
/* flags for non-system relation kinds to restrict use */
int restrict_nonsystem_relation_kind;
+/*
+ * Flag set by syscache listener to indicate if the user's password validity
+ * (rolvaliduntil) needs to be checked for expiration before the next
+ * command execution.
+ */
+static bool AuthCheckNeeded = false;
+
/* ----------------
* private typedefs etc
* ----------------
@@ -163,6 +173,13 @@ static volatile sig_atomic_t RecoveryConflictPendingReasons[NUM_PROCSIGNALS];
static MemoryContext row_description_context = NULL;
static StringInfoData row_description_buf;
+/*
+ * Tracks whether the SysCache callback for AUTHOID has been registered.
+ * This ensures CacheRegisterSyscacheCallback is called exactly once during
+ * backend initialization, preventing redundant registrations in the main loop.
+ */
+static bool password_auth_cache_callback_registered = false;
+
/* ----------------------------------------------------------------
* decls for routines only used in this file
* ----------------------------------------------------------------
@@ -186,6 +203,8 @@ static void drop_unnamed_stmt(void);
static void log_disconnections(int code, Datum arg);
static void enable_statement_timeout(void);
static void disable_statement_timeout(void);
+static void AuthCacheInvalidated(Datum arg, int cacheid, uint32 hashvalue);
+static void enforce_password_expiration(void);
/* ----------------------------------------------------------------
@@ -1050,6 +1069,11 @@ exec_simple_query(const char *query_string)
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/*
* Zap any pre-existing unnamed statement. (While not strictly necessary,
* it seems best to define simple-Query mode as if it used the unnamed
@@ -1432,6 +1456,11 @@ exec_parse_message(const char *query_string, /* string to execute */
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/*
* Switch to appropriate context for constructing parsetrees.
*
@@ -1708,6 +1737,11 @@ exec_bind_message(StringInfo input_message)
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/* Switch back to message context */
MemoryContextSwitchTo(MessageContext);
@@ -2221,6 +2255,11 @@ exec_execute_message(const char *portal_name, long max_rows)
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/*
* If we re-issue an Execute protocol request against an existing portal,
* then we are only fetching more rows rather than completely re-executing
@@ -2654,6 +2693,11 @@ exec_describe_statement_message(const char *stmt_name)
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/* Switch back to message context */
MemoryContextSwitchTo(MessageContext);
@@ -2747,6 +2791,11 @@ exec_describe_portal_message(const char *portal_name)
*/
start_xact_command();
+ /*
+ * Verify that the user's password has not expired.
+ */
+ enforce_password_expiration();
+
/* Switch back to message context */
MemoryContextSwitchTo(MessageContext);
@@ -4518,6 +4567,20 @@ PostgresMain(const char *dbname, const char *username)
if (!ignore_till_sync)
send_ready_for_query = true; /* initially, or after error */
+
+ /*
+ * Register a SysCache listener for pg_authid changes (specifically for
+ * rolvaliduntil). This provides an event-driven mechanism to enforce
+ * password/authorization expiration immediately upon change, rather than
+ * relying on polling. The callback sets a flag (AuthCheckNeeded) which
+ * is checked before executing each simple query.
+ */
+ if (!password_auth_cache_callback_registered)
+ {
+ CacheRegisterSyscacheCallback(AUTHOID, AuthCacheInvalidated, (Datum) 0);
+ password_auth_cache_callback_registered = true;
+ }
+
/*
* Non-error queries loop here.
*/
@@ -5237,3 +5300,92 @@ disable_statement_timeout(void)
if (get_timeout_active(STATEMENT_TIMEOUT))
disable_timeout(STATEMENT_TIMEOUT, false);
}
+
+/*
+ * CheckPasswordExpiration
+ * Refreshes the cached password expiration timestamp from the system cache.
+ * This function looks up the current user's entry in pg_authid and updates
+ * 'password_valid_until_timestamp' with the current value of 'rolvaliduntil'.
+ * It is called by enforce_password_expiration() when the 'AuthCheckNeeded'
+ * flag is set, typically due to a syscache invalidation (AuthCacheInvalidated).
+ */
+static void
+CheckPasswordExpiration(void)
+{
+ HeapTuple tuple;
+
+ /*
+ * Look up the current user's entry in pg_authid. We must do this, even
+ * if only AuthCheckNeeded is set, because GetUserId() might return a
+ * different user ID than the one that triggered the invalidation (though
+ * that's unlikely for AUTHOID).
+ */
+
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId()));
+
+ if (HeapTupleIsValid(tuple))
+ {
+ Datum rolvaliduntil_datum;
+ bool validUntil_null;
+
+ /* Get the expiration time column */
+ rolvaliduntil_datum = SysCacheGetAttr(AUTHNAME, tuple,
+ Anum_pg_authid_rolvaliduntil,
+ &validUntil_null);
+
+ if (!validUntil_null)
+ password_valid_until_timestamp = DatumGetTimestampTz(rolvaliduntil_datum);
+ else
+ password_valid_until_timestamp = 0;
+
+ ReleaseSysCache(tuple);
+ }
+ /* Reset the flag after performing the check */
+ AuthCheckNeeded = false;
+}
+
+/*
+ * enforce_password_expiration
+ *
+ * Check if the user's password has expired and terminate the connection
+ * if necessary. This encapsulates the state checks and the FATAL report.
+ * CheckPasswordExpiration must only be called when the system is out of
+ * recovery and inside a valid transaction.
+ */
+static void
+enforce_password_expiration(void)
+{
+
+ if (!RecoveryInProgress() && IsTransactionState() &&
+ password_valid_until_timestamp > 0)
+ {
+ if (AuthCheckNeeded)
+ CheckPasswordExpiration();
+
+ if (password_valid_until_timestamp < GetCurrentTransactionStartTimestamp())
+ ereport(FATAL,
+ (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
+ errmsg("Connection expired due to internal password policy enforcement"),
+ errdetail("User's password expired at %s.",
+ timestamptz_to_str(password_valid_until_timestamp)),
+ errhint("Reconnect with a renewed password.")));
+ }
+}
+
+/*
+ * AuthCacheInvalidated
+ * Syscache callback function registered for the AUTHOID cache (pg_authid).
+ *
+ * This function is executed whenever a tuple in pg_authid is updated, inserted,
+ * or deleted. Its primary purpose is to catch changes to the currently
+ * connected user's 'rolvaliduntil' field.
+ *
+ * It sets the static flag AuthCheckNeeded to true, signaling that the user's
+ * password expiration status must be checked.
+ */
+static void
+AuthCacheInvalidated(Datum arg, int cacheid, uint32 hashvalue)
+{
+ /* This callback is executed when an entry in pg_authid changes */
+ AuthCheckNeeded = true;
+}
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 36ad708b360..069138ec1d5 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -165,3 +165,10 @@ int notify_buffers = 16;
int serializable_buffers = 32;
int subtransaction_buffers = 0;
int transaction_buffers = 0;
+
+/*
+ * Cached value of the current user's password expiration time (pg_authid.rolvaliduntil).
+ * This value is updated via CheckPasswordExpiration() when the AuthCheckNeeded
+ * flag is set by a syscache invalidation callback.
+ */
+TimestampTz password_valid_until_timestamp = -1;
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index db559b39c4d..146c3769b2f 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -287,7 +287,7 @@ extern PGDLLIMPORT double VacuumCostDelay;
extern PGDLLIMPORT int VacuumCostBalance;
extern PGDLLIMPORT bool VacuumCostActive;
-
+extern PGDLLIMPORT TimestampTz password_valid_until_timestamp;
/* in utils/misc/stack_depth.c */
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Periodic authorization expiration checks using GoAway message
@ 2026-01-22 07:40 Zsolt Parragi <[email protected]>
parent: Ajit Awekar <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Zsolt Parragi @ 2026-01-22 07:40 UTC (permalink / raw)
To: Ajit Awekar <[email protected]>; +Cc: Jacob Champion <[email protected]>; Jelte Fennema-Nio <[email protected]>; Hannu Krosing <[email protected]>; pgsql-hackers; Dave Cramer <[email protected]>; Heikki Linnakangas <[email protected]>
Hello!
> Done. I have modified the condition check so as it will not impact users
> having rolvaliduntil to NULL.
Thanks! After one more look at this, shouldn't we refresh the cache if
AuthCheckNeeded, regardless of the valid until timestamp value? (for
example by moving the >0 condition to the inner if)
Consider the following scenario:
1. user logs in, without a valid until date set
2. valid until is set to something
3. existing session started in 1 will never be terminated
postgres.c:5368 - I missed this in my previos review, the error
message should start with a lowercase character.
> This patch introduces a mechanism to address the security issue of stale,
> authorized connections persisting beyond their validity period. .
postgres.c:5326 - if the user was dropped, but the connection is still
active, the patch silently ignores it. That matches the current
behavior of postgres, but is that the expected behavior in the context
of this patch?
^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v1 7/8] convert ParallelBlockTableScanDescData->phs_{start,num}block to atomics
@ 2026-07-09 20:21 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Bossart @ 2026-07-09 20:21 UTC (permalink / raw)
---
src/backend/access/heap/heapam_handler.c | 2 +-
src/backend/access/table/tableam.c | 59 +++++++++++-------------
src/include/access/relscan.h | 8 ++--
3 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bf87430cf01..0f24a132564 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1965,7 +1965,7 @@ heapam_scan_get_blocks_done(HeapScanDesc hscan)
if (hscan->rs_base.rs_parallel != NULL)
{
bpscan = (ParallelBlockTableScanDesc) hscan->rs_base.rs_parallel;
- startblock = bpscan->phs_startblock;
+ startblock = pg_atomic_read_u32(&bpscan->phs_startblock);
}
else
startblock = hscan->rs_startblock;
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index 68ff0966f1c..f2038ea9205 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -421,9 +421,8 @@ table_block_parallelscan_initialize(Relation rel, ParallelTableScanDesc pscan)
bpscan->base.phs_syncscan = synchronize_seqscans &&
!RelationUsesLocalBuffers(rel) &&
bpscan->phs_nblocks > NBuffers / 4;
- SpinLockInit(&bpscan->phs_mutex);
- bpscan->phs_startblock = InvalidBlockNumber;
- bpscan->phs_numblock = InvalidBlockNumber;
+ pg_atomic_init_u32(&bpscan->phs_startblock, InvalidBlockNumber);
+ pg_atomic_init_u32(&bpscan->phs_numblock, InvalidBlockNumber);
pg_atomic_init_u64(&bpscan->phs_nallocated, 0);
return sizeof(ParallelBlockTableScanDescData);
@@ -459,25 +458,22 @@ table_block_parallelscan_startblock_init(Relation rel,
StaticAssertDecl(MaxBlockNumber <= 0xFFFFFFFE,
"pg_nextpower2_32 may be too small for non-standard BlockNumber width");
- BlockNumber sync_startpage = InvalidBlockNumber;
BlockNumber scan_nblocks;
/* Reset the state we use for controlling allocation size. */
memset(pbscanwork, 0, sizeof(*pbscanwork));
-retry:
- /* Grab the spinlock. */
- SpinLockAcquire(&pbscan->phs_mutex);
-
/*
* When the caller specified a limit on the number of blocks to scan, set
* that in the ParallelBlockTableScanDesc, if it's not been done by
* another worker already.
*/
- if (numblocks != InvalidBlockNumber &&
- pbscan->phs_numblock == InvalidBlockNumber)
+ if (numblocks != InvalidBlockNumber)
{
- pbscan->phs_numblock = numblocks;
+ uint32 expected = InvalidBlockNumber;
+
+ pg_atomic_compare_exchange_u32(&pbscan->phs_numblock, &expected,
+ numblocks);
}
/*
@@ -485,36 +481,35 @@ retry:
* so now. If a startblock was specified, start there, otherwise if this
* is not a synchronized scan, we just start at block 0, but if it is a
* synchronized scan, we must get the starting position from the
- * synchronized scan machinery. We can't hold the spinlock while doing
- * that, though, so release the spinlock, get the information we need, and
- * retry. If nobody else has initialized the scan in the meantime, we'll
- * fill in the value we fetched on the second time through.
+ * synchronized scan machinery.
+ *
+ * If another worker initializes phs_startblock concurrently, just use
+ * their value.
*/
- if (pbscan->phs_startblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_startblock) == InvalidBlockNumber)
{
+ BlockNumber newstartblock;
+ uint32 expected = InvalidBlockNumber;
+
if (startblock != InvalidBlockNumber)
- pbscan->phs_startblock = startblock;
+ newstartblock = startblock;
else if (!pbscan->base.phs_syncscan)
- pbscan->phs_startblock = 0;
- else if (sync_startpage != InvalidBlockNumber)
- pbscan->phs_startblock = sync_startpage;
+ newstartblock = 0;
else
- {
- SpinLockRelease(&pbscan->phs_mutex);
- sync_startpage = ss_get_location(rel, pbscan->phs_nblocks);
- goto retry;
- }
+ newstartblock = ss_get_location(rel, pbscan->phs_nblocks);
+
+ pg_atomic_compare_exchange_u32(&pbscan->phs_startblock, &expected,
+ newstartblock);
}
- SpinLockRelease(&pbscan->phs_mutex);
/*
* Figure out how many blocks we're going to scan; either all of them, or
* just phs_numblock's worth, if a limit has been imposed.
*/
- if (pbscan->phs_numblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_numblock) == InvalidBlockNumber)
scan_nblocks = pbscan->phs_nblocks;
else
- scan_nblocks = pbscan->phs_numblock;
+ scan_nblocks = pg_atomic_read_u32(&pbscan->phs_numblock);
/*
* We determine the chunk size based on scan_nblocks. First we split
@@ -595,10 +590,10 @@ table_block_parallelscan_nextpage(Relation rel,
*/
/* First, figure out how many blocks we're planning on scanning */
- if (pbscan->phs_numblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_numblock) == InvalidBlockNumber)
scan_nblocks = pbscan->phs_nblocks;
else
- scan_nblocks = pbscan->phs_numblock;
+ scan_nblocks = pg_atomic_read_u32(&pbscan->phs_numblock);
/*
* Now check if we have any remaining blocks in a previous chunk for this
@@ -644,7 +639,7 @@ table_block_parallelscan_nextpage(Relation rel,
if (nallocated >= scan_nblocks)
page = InvalidBlockNumber; /* all blocks have been allocated */
else
- page = (nallocated + pbscan->phs_startblock) % pbscan->phs_nblocks;
+ page = (nallocated + pg_atomic_read_u32(&pbscan->phs_startblock)) % pbscan->phs_nblocks;
/*
* Report scan location. Normally, we report the current page number.
@@ -658,7 +653,7 @@ table_block_parallelscan_nextpage(Relation rel,
if (page != InvalidBlockNumber)
ss_report_location(rel, page);
else if (nallocated == pbscan->phs_nblocks)
- ss_report_location(rel, pbscan->phs_startblock);
+ ss_report_location(rel, pg_atomic_read_u32(&pbscan->phs_startblock));
}
return page;
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 2ea06a67a63..2305d0159f3 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -19,7 +19,6 @@
#include "nodes/tidbitmap.h"
#include "port/atomics.h"
#include "storage/relfilelocator.h"
-#include "storage/spin.h"
#include "utils/relcache.h"
@@ -99,10 +98,9 @@ typedef struct ParallelBlockTableScanDescData
ParallelTableScanDescData base;
BlockNumber phs_nblocks; /* # blocks in relation at start of scan */
- slock_t phs_mutex; /* mutual exclusion for setting startblock */
- BlockNumber phs_startblock; /* starting block number */
- BlockNumber phs_numblock; /* # blocks to scan, or InvalidBlockNumber if
- * no limit */
+ pg_atomic_uint32 phs_startblock; /* starting block number */
+ pg_atomic_uint32 phs_numblock; /* # blocks to scan, or InvalidBlockNumber
+ * if no limit */
pg_atomic_uint64 phs_nallocated; /* number of blocks allocated to
* workers so far. */
} ParallelBlockTableScanDescData;
--
2.50.1 (Apple Git-155)
--Ot5x3ffkWEuxilWO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v1-0008-convert-FastPathStrongRelationLocks-to-atomics.patch
^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v1 7/8] convert ParallelBlockTableScanDescData->phs_{start,num}block to atomics
@ 2026-07-09 20:21 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Nathan Bossart @ 2026-07-09 20:21 UTC (permalink / raw)
---
src/backend/access/heap/heapam_handler.c | 2 +-
src/backend/access/table/tableam.c | 59 +++++++++++-------------
src/include/access/relscan.h | 8 ++--
3 files changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index bf87430cf01..0f24a132564 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -1965,7 +1965,7 @@ heapam_scan_get_blocks_done(HeapScanDesc hscan)
if (hscan->rs_base.rs_parallel != NULL)
{
bpscan = (ParallelBlockTableScanDesc) hscan->rs_base.rs_parallel;
- startblock = bpscan->phs_startblock;
+ startblock = pg_atomic_read_u32(&bpscan->phs_startblock);
}
else
startblock = hscan->rs_startblock;
diff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index 68ff0966f1c..f2038ea9205 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -421,9 +421,8 @@ table_block_parallelscan_initialize(Relation rel, ParallelTableScanDesc pscan)
bpscan->base.phs_syncscan = synchronize_seqscans &&
!RelationUsesLocalBuffers(rel) &&
bpscan->phs_nblocks > NBuffers / 4;
- SpinLockInit(&bpscan->phs_mutex);
- bpscan->phs_startblock = InvalidBlockNumber;
- bpscan->phs_numblock = InvalidBlockNumber;
+ pg_atomic_init_u32(&bpscan->phs_startblock, InvalidBlockNumber);
+ pg_atomic_init_u32(&bpscan->phs_numblock, InvalidBlockNumber);
pg_atomic_init_u64(&bpscan->phs_nallocated, 0);
return sizeof(ParallelBlockTableScanDescData);
@@ -459,25 +458,22 @@ table_block_parallelscan_startblock_init(Relation rel,
StaticAssertDecl(MaxBlockNumber <= 0xFFFFFFFE,
"pg_nextpower2_32 may be too small for non-standard BlockNumber width");
- BlockNumber sync_startpage = InvalidBlockNumber;
BlockNumber scan_nblocks;
/* Reset the state we use for controlling allocation size. */
memset(pbscanwork, 0, sizeof(*pbscanwork));
-retry:
- /* Grab the spinlock. */
- SpinLockAcquire(&pbscan->phs_mutex);
-
/*
* When the caller specified a limit on the number of blocks to scan, set
* that in the ParallelBlockTableScanDesc, if it's not been done by
* another worker already.
*/
- if (numblocks != InvalidBlockNumber &&
- pbscan->phs_numblock == InvalidBlockNumber)
+ if (numblocks != InvalidBlockNumber)
{
- pbscan->phs_numblock = numblocks;
+ uint32 expected = InvalidBlockNumber;
+
+ pg_atomic_compare_exchange_u32(&pbscan->phs_numblock, &expected,
+ numblocks);
}
/*
@@ -485,36 +481,35 @@ retry:
* so now. If a startblock was specified, start there, otherwise if this
* is not a synchronized scan, we just start at block 0, but if it is a
* synchronized scan, we must get the starting position from the
- * synchronized scan machinery. We can't hold the spinlock while doing
- * that, though, so release the spinlock, get the information we need, and
- * retry. If nobody else has initialized the scan in the meantime, we'll
- * fill in the value we fetched on the second time through.
+ * synchronized scan machinery.
+ *
+ * If another worker initializes phs_startblock concurrently, just use
+ * their value.
*/
- if (pbscan->phs_startblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_startblock) == InvalidBlockNumber)
{
+ BlockNumber newstartblock;
+ uint32 expected = InvalidBlockNumber;
+
if (startblock != InvalidBlockNumber)
- pbscan->phs_startblock = startblock;
+ newstartblock = startblock;
else if (!pbscan->base.phs_syncscan)
- pbscan->phs_startblock = 0;
- else if (sync_startpage != InvalidBlockNumber)
- pbscan->phs_startblock = sync_startpage;
+ newstartblock = 0;
else
- {
- SpinLockRelease(&pbscan->phs_mutex);
- sync_startpage = ss_get_location(rel, pbscan->phs_nblocks);
- goto retry;
- }
+ newstartblock = ss_get_location(rel, pbscan->phs_nblocks);
+
+ pg_atomic_compare_exchange_u32(&pbscan->phs_startblock, &expected,
+ newstartblock);
}
- SpinLockRelease(&pbscan->phs_mutex);
/*
* Figure out how many blocks we're going to scan; either all of them, or
* just phs_numblock's worth, if a limit has been imposed.
*/
- if (pbscan->phs_numblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_numblock) == InvalidBlockNumber)
scan_nblocks = pbscan->phs_nblocks;
else
- scan_nblocks = pbscan->phs_numblock;
+ scan_nblocks = pg_atomic_read_u32(&pbscan->phs_numblock);
/*
* We determine the chunk size based on scan_nblocks. First we split
@@ -595,10 +590,10 @@ table_block_parallelscan_nextpage(Relation rel,
*/
/* First, figure out how many blocks we're planning on scanning */
- if (pbscan->phs_numblock == InvalidBlockNumber)
+ if (pg_atomic_read_u32(&pbscan->phs_numblock) == InvalidBlockNumber)
scan_nblocks = pbscan->phs_nblocks;
else
- scan_nblocks = pbscan->phs_numblock;
+ scan_nblocks = pg_atomic_read_u32(&pbscan->phs_numblock);
/*
* Now check if we have any remaining blocks in a previous chunk for this
@@ -644,7 +639,7 @@ table_block_parallelscan_nextpage(Relation rel,
if (nallocated >= scan_nblocks)
page = InvalidBlockNumber; /* all blocks have been allocated */
else
- page = (nallocated + pbscan->phs_startblock) % pbscan->phs_nblocks;
+ page = (nallocated + pg_atomic_read_u32(&pbscan->phs_startblock)) % pbscan->phs_nblocks;
/*
* Report scan location. Normally, we report the current page number.
@@ -658,7 +653,7 @@ table_block_parallelscan_nextpage(Relation rel,
if (page != InvalidBlockNumber)
ss_report_location(rel, page);
else if (nallocated == pbscan->phs_nblocks)
- ss_report_location(rel, pbscan->phs_startblock);
+ ss_report_location(rel, pg_atomic_read_u32(&pbscan->phs_startblock));
}
return page;
diff --git a/src/include/access/relscan.h b/src/include/access/relscan.h
index 2ea06a67a63..2305d0159f3 100644
--- a/src/include/access/relscan.h
+++ b/src/include/access/relscan.h
@@ -19,7 +19,6 @@
#include "nodes/tidbitmap.h"
#include "port/atomics.h"
#include "storage/relfilelocator.h"
-#include "storage/spin.h"
#include "utils/relcache.h"
@@ -99,10 +98,9 @@ typedef struct ParallelBlockTableScanDescData
ParallelTableScanDescData base;
BlockNumber phs_nblocks; /* # blocks in relation at start of scan */
- slock_t phs_mutex; /* mutual exclusion for setting startblock */
- BlockNumber phs_startblock; /* starting block number */
- BlockNumber phs_numblock; /* # blocks to scan, or InvalidBlockNumber if
- * no limit */
+ pg_atomic_uint32 phs_startblock; /* starting block number */
+ pg_atomic_uint32 phs_numblock; /* # blocks to scan, or InvalidBlockNumber
+ * if no limit */
pg_atomic_uint64 phs_nallocated; /* number of blocks allocated to
* workers so far. */
} ParallelBlockTableScanDescData;
--
2.50.1 (Apple Git-155)
--Ot5x3ffkWEuxilWO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename=v1-0008-convert-FastPathStrongRelationLocks-to-atomics.patch
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-07-09 20:21 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-01-20 09:28 Re: Periodic authorization expiration checks using GoAway message Zsolt Parragi <[email protected]>
2026-01-20 12:31 ` Ajit Awekar <[email protected]>
2026-01-22 07:40 ` Zsolt Parragi <[email protected]>
2026-07-09 20:21 [PATCH v1 7/8] convert ParallelBlockTableScanDescData->phs_{start,num}block to atomics Nathan Bossart <[email protected]>
2026-07-09 20:21 [PATCH v1 7/8] convert ParallelBlockTableScanDescData->phs_{start,num}block to atomics Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox