From f21ade63e94d94653f030ff8e176856486a26c9c Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Thu, 10 Sep 2026 11:09:49 -0400 Subject: [PATCH v6 3/5] pg_get_role_ddl, pg_get_tablespace_ddl, pg_get_database_ddl: allow suppressing IN DATABASE settings, fix concurrent-drop races Add in_database_settings (default true) to pg_get_role_ddl(), so a caller assembling a role's DDL before any database exists can ask for just the role-wide ALTER ROLE SET statements, without the IN DATABASE ones that depend on the database already existing. Fix a concurrent-drop race shared by all three functions: each looked up its target by OID, then made later catalog scans keyed by that OID without holding a lock in between. A concurrent DROP after the lookup left those scans finding no rows -- since DROP cleans up dependents too -- so the function silently returned an incomplete result instead of erroring. Fix by calling shdepLockAndCheckObject() right after the initial lookup, in all three functions. Reported-by: Noah Misch Reported-by: Japin Li Discussion: https://postgr.es/m/20260827015242.54.noahmisch@microsoft.com Discussion: https://postgr.es/m/MEAPR01MB3031C47538F35A6C25B20DA5B682A@MEAPR01MB3031.ausprd01.prod.outlook.com --- src/backend/utils/adt/ddlutils.c | 49 ++++++++++++++++++++++++-------- src/include/catalog/pg_proc.dat | 8 +++--- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/backend/utils/adt/ddlutils.c b/src/backend/utils/adt/ddlutils.c index 8343cc5614f..64258776bb8 100644 --- a/src/backend/utils/adt/ddlutils.c +++ b/src/backend/utils/adt/ddlutils.c @@ -21,6 +21,7 @@ #include "access/genam.h" #include "access/htup_details.h" #include "access/table.h" +#include "catalog/dependency.h" #include "catalog/pg_auth_members.h" #include "catalog/pg_authid.h" #include "catalog/pg_collation.h" @@ -50,7 +51,8 @@ static void append_ddl_option(StringInfo buf, bool pretty, int indent, static void append_guc_value(StringInfo buf, const char *name, const char *value); static List *pg_get_role_ddl_internal(Oid roleid, bool pretty, - bool memberships, bool password); + bool memberships, bool password, + bool in_database_settings); static List *pg_get_tablespace_ddl_internal(Oid tsid, bool pretty, bool no_owner); static Datum pg_get_tablespace_ddl_srf(FunctionCallInfo fcinfo, Oid tsid); static List *pg_get_database_ddl_internal(Oid dbid, bool pretty, @@ -139,16 +141,13 @@ append_guc_value(StringInfo buf, const char *name, const char *value) * Generate DDL statements to recreate a role * * Returns a List of palloc'd strings, each being a complete SQL statement. - * The first two list elements are always the CREATE ROLE statement and an - * ALTER ROLE statement carrying the role attributes; subsequent elements are - * ALTER ROLE SET statements for any role-specific or role-in-database - * configuration settings. If memberships is true, GRANT statements for - * role memberships are appended. If password is false, the PASSWORD - * clause is omitted. + * The first two elements are the CREATE ROLE statement and an ALTER ROLE + * carrying the attributes; the rest are ALTER ROLE SET statements, plus + * GRANT statements for memberships if requested. */ static List * pg_get_role_ddl_internal(Oid roleid, bool pretty, bool memberships, - bool password) + bool password, bool in_database_settings) { HeapTuple tuple; Form_pg_authid roleform; @@ -171,6 +170,12 @@ pg_get_role_ddl_internal(Oid roleid, bool pretty, bool memberships, roleform = (Form_pg_authid) GETSTRUCT(tuple); rolname = pstrdup(NameStr(roleform->rolname)); + /* + * Lock and re-verify existence, closing the window for a concurrent + * DROP ROLE before the scans below. + */ + shdepLockAndCheckObject(AuthIdRelationId, roleid); + /* * rolpassword needs SELECT on pg_authid; nothing else here is * sensitive, so only check it when the PASSWORD clause is wanted. @@ -314,11 +319,16 @@ pg_get_role_ddl_internal(Oid roleid, bool pretty, bool memberships, /* * If setdatabase is valid, this is a role-in-database setting; - * otherwise it's a role-wide setting. Look up the database name once - * for all settings in this row. + * otherwise it's a role-wide setting. In-database settings depend on + * that database already existing, so they're optional; role-wide ones + * are not. Look up the database name once for all settings in this + * row. */ if (OidIsValid(datid)) { + if (!in_database_settings) + continue; + datname = get_database_name(datid); /* Database has been dropped; skip all settings in this row. */ if (datname == NULL) @@ -471,6 +481,7 @@ pg_get_role_ddl(PG_FUNCTION_ARGS) bool pretty; bool memberships; bool password; + bool in_database_settings; funcctx = SRF_FIRSTCALL_INIT(); oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); @@ -479,9 +490,10 @@ pg_get_role_ddl(PG_FUNCTION_ARGS) pretty = PG_GETARG_BOOL(1); memberships = PG_GETARG_BOOL(2); password = PG_GETARG_BOOL(3); + in_database_settings = PG_GETARG_BOOL(4); statements = pg_get_role_ddl_internal(roleid, pretty, memberships, - password); + password, in_database_settings); funcctx->user_fctx = statements; funcctx->max_calls = list_length(statements); @@ -537,7 +549,14 @@ pg_get_tablespace_ddl_internal(Oid tsid, bool pretty, bool no_owner) tspForm = (Form_pg_tablespace) GETSTRUCT(tuple); spcname = pstrdup(NameStr(tspForm->spcname)); - /* User must have SELECT privilege on pg_tablespace. */ + /* Guard against a concurrent DROP TABLESPACE, as for roles/databases. */ + shdepLockAndCheckObject(TableSpaceRelationId, tsid); + + /* + * User must have SELECT privilege on pg_tablespace. As in + * pg_get_role_ddl_internal(), the error below names the tablespace, not + * pg_tablespace, for the same reason. + */ if (pg_class_aclcheck(TableSpaceRelationId, GetUserId(), ACL_SELECT) != ACLCHECK_OK) { ReleaseSysCache(tuple); @@ -744,6 +763,12 @@ pg_get_database_ddl_internal(Oid dbid, bool pretty, dbform = (Form_pg_database) GETSTRUCT(tuple); dbname = pstrdup(NameStr(dbform->datname)); + /* + * Lock and re-verify existence, closing the window for a concurrent + * DROP DATABASE before the scan below. + */ + shdepLockAndCheckObject(DatabaseRelationId, dbid); + /* * Reject invalid databases. Deparsing a pg_database row in invalid state * can produce SQL that is not executable, such as CONNECTION LIMIT = -2. diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 9595e21b3eb..4db14a008ba 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -8669,10 +8669,10 @@ proargtypes => 'oid bool', prosrc => 'pg_get_constraintdef_ext' }, { oid => '6501', descr => 'get DDL to recreate a role', proname => 'pg_get_role_ddl', prorows => '10', proretset => 't', - provolatile => 's', pronargdefaults => '3', prorettype => 'text', - proargtypes => 'regrole bool bool bool', - proargnames => '{role,pretty,memberships,password}', - proargdefaults => '{false,true,true}', prosrc => 'pg_get_role_ddl' }, + provolatile => 's', pronargdefaults => '4', prorettype => 'text', + proargtypes => 'regrole bool bool bool bool', + proargnames => '{role,pretty,memberships,password,in_database_settings}', + proargdefaults => '{false,true,true,true}', prosrc => 'pg_get_role_ddl' }, { oid => '6499', descr => 'get DDL to recreate a tablespace', proname => 'pg_get_tablespace_ddl', prorows => '10', proretset => 't', provolatile => 's', pronargdefaults => '2', prorettype => 'text', -- 2.43.0