public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v13 06/18] More refactoring 5+ messages / 3 participants [nested] [flat]
* [PATCH v13 06/18] More refactoring @ 2020-11-01 19:46 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-11-01 19:46 UTC (permalink / raw) --- src/backend/commands/indexcmds.c | 201 ++++++++++----------- src/test/regress/expected/create_index.out | 4 +- 2 files changed, 93 insertions(+), 112 deletions(-) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index f5fea14ff4..5c5596cd28 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -102,8 +102,8 @@ static void ReindexMultipleInternal(List *relids, ReindexParams *params); static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params); -static List *ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, - ReindexParams *params, MemoryContext private_context); +static List *ReindexIndexesConcurrently(List *indexIds, ReindexParams *params, + MemoryContext private_context); static void update_relispartition(Oid relationId, bool newval); static inline void set_indexsafe_procflags(void); @@ -2652,8 +2652,8 @@ ReindexIndex(RangeVar *indexRelation, ReindexParams *params, bool isTopLevel) .indexId = indOid, /* other fields set later */ }; + ReindexIndexesConcurrently(list_make1(&idxinfo), - list_make1_oid(IndexGetRelation(indOid, false)), params, CurrentMemoryContext); } else @@ -3023,12 +3023,11 @@ reindex_error_callback(void *arg) /* - * Given a list of index oids, return a list of leaf partitions by removing - * any intermediate parents. heaprels is populated with the corresponding - * tables. + * Given a list of index oids, return a new list of leaf partitions by + * excluding any intermediate parents. */ static List * -leaf_indexes(List *inhoids, int options, List **heaprels) +leaf_indexes(List *inhoids, int options) { List *partitions = NIL; ListCell *lc; @@ -3060,7 +3059,6 @@ leaf_indexes(List *inhoids, int options, List **heaprels) /* Save partition OID in current MemoryContext */ partitions = lappend_oid(partitions, partoid); - *heaprels = lappend_oid(*heaprels, tableoid); } return partitions; @@ -3072,13 +3070,11 @@ leaf_indexes(List *inhoids, int options, List **heaprels) * * Reindex a set of partitions, per the partitioned index or table given * by the caller. - * XXX: should be further refactored with logic from ReindexRelationConcurrently */ static void ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) { - List *partitions = NIL, - *heaprels = NIL; + List *partitions = NIL; char relkind = get_rel_relkind(relid); char *relname = get_rel_name(relid); char *relnamespace = get_namespace_name(get_rel_namespace(relid)); @@ -3132,7 +3128,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) if (relkind == RELKIND_PARTITIONED_INDEX) { old_context = MemoryContextSwitchTo(reindex_context); - partitions = leaf_indexes(inhoids, params->options, &heaprels); + partitions = leaf_indexes(inhoids, params->options); MemoryContextSwitchTo(old_context); } else { /* Loop over parent tables */ @@ -3145,7 +3141,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) parttable = table_open(partoid, ShareLock); old_context = MemoryContextSwitchTo(reindex_context); partindexes = RelationGetIndexList(parttable); - partindexes = leaf_indexes(partindexes, params->options, &heaprels); + partindexes = leaf_indexes(partindexes, params->options); partitions = list_concat(partitions, partindexes); MemoryContextSwitchTo(old_context); @@ -3154,10 +3150,9 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) } if ((params->options & REINDEXOPT_CONCURRENTLY) != 0 && - relkind == RELKIND_PARTITIONED_INDEX && get_rel_persistence(relid) != RELPERSISTENCE_TEMP) { - List *idxinfos = NIL; + List *idxinfos = NIL; ReindexIndexInfo *idxinfo; old_context = MemoryContextSwitchTo(reindex_context); @@ -3172,7 +3167,7 @@ ReindexPartitions(Oid relid, ReindexParams *params, bool isTopLevel) MemoryContextSwitchTo(old_context); /* Process all indexes in a single loop */ - ReindexIndexesConcurrently(idxinfos, heaprels, params, reindex_context); + ReindexIndexesConcurrently(idxinfos, params, reindex_context); } else { /* * Process each partition listed in a separate transaction. Note that @@ -3342,7 +3337,6 @@ ReindexMultipleInternal(List *relids, ReindexParams *params) static bool ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) { - List *heapRelationIds = NIL; List *indexIds = NIL; List *newIndexIds = NIL; ListCell *lc, @@ -3395,14 +3389,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) */ Relation heapRelation; - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track this relation for session locks */ - heapRelationIds = lappend_oid(heapRelationIds, relationOid); - - MemoryContextSwitchTo(oldcontext); - if (IsCatalogRelationOid(relationOid)) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), @@ -3415,7 +3401,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) ShareUpdateExclusiveLock); /* leave if relation does not exist */ if (!heapRelation) - break; + break; // XXX: lremove } else heapRelation = table_open(relationOid, @@ -3473,14 +3459,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) Relation toastRelation = table_open(toastOid, ShareUpdateExclusiveLock); - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track this relation for session locks */ - heapRelationIds = lappend_oid(heapRelationIds, toastOid); - - MemoryContextSwitchTo(oldcontext); - foreach(lc2, RelationGetIndexList(toastRelation)) { Oid cellOid = lfirst_oid(lc2); @@ -3521,78 +3499,6 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) break; } case RELKIND_INDEX: - { - Oid heapId = IndexGetRelation(relationOid, - (params->options & REINDEXOPT_MISSING_OK) != 0); - Relation heapRelation; - ReindexIndexInfo *idx; - - /* if relation is missing, leave */ - if (!OidIsValid(heapId)) - break; - - if (IsCatalogRelationOid(heapId)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot reindex system catalogs concurrently"))); - - /* - * Don't allow reindex for an invalid index on TOAST table, as - * if rebuilt it would not be possible to drop it. Match - * error message in reindex_index(). - */ - if (IsToastNamespace(get_rel_namespace(relationOid)) && - !get_index_isvalid(relationOid)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot reindex invalid index on TOAST table"))); - - /* - * Check if parent relation can be locked and if it exists, - * this needs to be done at this stage as the list of indexes - * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK - * should not be used once all the session locks are taken. - */ - if ((params->options & REINDEXOPT_MISSING_OK) != 0) - { - heapRelation = try_table_open(heapId, - ShareUpdateExclusiveLock); - /* leave if relation does not exist */ - if (!heapRelation) - break; - } - else - heapRelation = table_open(heapId, - ShareUpdateExclusiveLock); - - if (OidIsValid(params->tablespaceOid) && - IsSystemRelation(heapRelation)) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot move system relation \"%s\"", - get_rel_name(relationOid)))); - - table_close(heapRelation, NoLock); - - /* Save the list of relation OIDs in private context */ - oldcontext = MemoryContextSwitchTo(private_context); - - /* Track the heap relation of this index for session locks */ - heapRelationIds = list_make1_oid(heapId); - - /* - * Save the list of relation OIDs in private context. Note - * that invalid indexes are allowed here. - */ - idx = palloc(sizeof(ReindexIndexInfo)); - idx->indexId = relationOid; - indexIds = lappend(indexIds, idx); - /* other fields set later */ - - MemoryContextSwitchTo(oldcontext); - break; - } - case RELKIND_PARTITIONED_TABLE: case RELKIND_PARTITIONED_INDEX: default: @@ -3623,10 +3529,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) errmsg("cannot move non-shared relation to tablespace \"%s\"", get_tablespace_name(params->tablespaceOid)))); - Assert(heapRelationIds != NIL); + // Assert(heapRelationIds != NIL); /* Do the work */ - newIndexIds = ReindexIndexesConcurrently(indexIds, heapRelationIds, params, private_context); + newIndexIds = ReindexIndexesConcurrently(indexIds, params, private_context); /* Log what we did */ if ((params->options & REINDEXOPT_VERBOSE) != 0) @@ -3668,9 +3574,10 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params) * This is called by ReindexRelationConcurrently and */ static List * -ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, - ReindexParams *params, MemoryContext private_context) +ReindexIndexesConcurrently(List *indexIds, ReindexParams *params, + MemoryContext private_context) { + List *heapRelationIds = NIL; List *newIndexIds = NIL; List *relationLocks = NIL; List *lockTags = NIL; @@ -3688,6 +3595,80 @@ ReindexIndexesConcurrently(List *indexIds, List *heapRelationIds, }; int64 progress_vals[4]; + /* It's not a shared catalog, so refuse to move it to shared tablespace */ + if (params->tablespaceOid == GLOBALTABLESPACE_OID && false) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move non-shared relation to tablespace \"%s\"", + get_tablespace_name(params->tablespaceOid)))); + + foreach(lc, indexIds) + { + ReindexIndexInfo *idx = lfirst(lc); + Oid indexrelid = idx->indexId; + Oid heapId = IndexGetRelation(indexrelid, + (params->options & REINDEXOPT_MISSING_OK) != 0); + Relation heapRelation; + + /* if relation is missing, leave */ + if (!OidIsValid(heapId)) + break; // XXX: ldelete? + + if (IsCatalogRelationOid(heapId)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot reindex system catalogs concurrently"))); + + /* + * Don't allow reindex for an invalid index on TOAST table, as + * if rebuilt it would not be possible to drop it. Match + * error message in reindex_index(). + */ + if (IsToastNamespace(get_rel_namespace(indexrelid)) && + !get_index_isvalid(indexrelid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot reindex invalid index on TOAST table"))); + + if (OidIsValid(params->tablespaceOid) && + IsCatalogRelationOid(indexrelid)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot move system relation \"%s\"", + get_rel_name(indexrelid)))); + + /* + * Check if parent relation can be locked and if it exists, + * this needs to be done at this stage as the list of indexes + * to rebuild is not complete yet, and REINDEXOPT_MISSING_OK + * should not be used once all the session locks are taken. + */ + if ((params->options & REINDEXOPT_MISSING_OK) != 0) + { + heapRelation = try_table_open(heapId, + ShareUpdateExclusiveLock); + /* leave if relation does not exist */ + if (!heapRelation) + break; // ldelete + } + else + heapRelation = table_open(heapId, + ShareUpdateExclusiveLock); + table_close(heapRelation, NoLock); + + /* Save the list of relation OIDs in private context */ + oldcontext = MemoryContextSwitchTo(private_context); + + /* Track the heap relation of this index for session locks */ + heapRelationIds = lappend_oid(heapRelationIds, heapId); + // heapRelationIds = list_make1_oid(heapId); + + /* Note that invalid indexes are allowed here. */ + + MemoryContextSwitchTo(oldcontext); + // break; + } + /*----- * Now we have all the indexes we want to process in indexIds. * diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out index 6f41adf736..830fdddf24 100644 --- a/src/test/regress/expected/create_index.out +++ b/src/test/regress/expected/create_index.out @@ -2470,12 +2470,12 @@ COMMIT; REINDEX TABLE CONCURRENTLY pg_class; -- no catalog relation ERROR: cannot reindex system catalogs concurrently REINDEX INDEX CONCURRENTLY pg_class_oid_index; -- no catalog index -ERROR: concurrent index creation on system catalog tables is not supported +ERROR: cannot reindex system catalogs concurrently -- These are the toast table and index of pg_authid. REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_1260; -- no catalog toast table ERROR: cannot reindex system catalogs concurrently REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index -ERROR: concurrent index creation on system catalog tables is not supported +ERROR: cannot reindex system catalogs concurrently REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM ERROR: cannot reindex system catalogs concurrently -- Warns about catalog relations -- 2.17.0 --QTprm0S8XgL7H0Dt-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make MaxBackends available in _PG_init @ 2022-01-07 18:09 Bossart, Nathan <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Bossart, Nathan @ 2022-01-07 18:09 UTC (permalink / raw) To: Fujii Masao <[email protected]>; [email protected] <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]> On 1/7/22, 8:54 AM, "Fujii Masao" <[email protected]> wrote: > The patch handles only MaxBackends. But isn't there other variable having the same issue? I wouldn't be surprised to learn of other cases, but I've only encountered this specific issue with MaxBackends. I think MaxBackends is notable because it is more likely to be used by preloaded libraries but is intentionally initialized after loading them. As noted in an earlier message on this thread [0], using MaxBackends in a call to RequestAddinShmemSpace() in _PG_init() may not reliably cause problems, too. > It seems overkill to remove "extern" from MaxBackends and replace MaxBackends with GetMaxBackends() in the existing PostgreSQL codes. I'm not sure how much it's actually worth doing that. Instead, isn't it enough to just add the comment like "Use GetMaxBackends() if you want to treat the lookup for uninitialized MaxBackends as an error" in the definition of MaxBackends? While that approach would provide a way to safely retrieve the value, I think it would do little to prevent the issue in practice. If the size of the patch is a concern, we could also convert MaxBackends into a macro for calling GetMaxBackends(). This could also be a nice way to avoid breaking extensions that are using it correctly while triggering ERRORs for extensions that are using it incorrectly. I've attached a new version of the patch that does it this way. Nathan [0] https://postgr.es/m/8499D41B-628A-4CE0-9139-00D718F9D06B%40amazon.com Attachments: [application/octet-stream] v5-0001-Disallow-external-access-to-MaxBackends.patch (6.7K, ../../[email protected]/2-v5-0001-Disallow-external-access-to-MaxBackends.patch) download | inline diff: From f416355c8a73dd7f0879f4f78666499ceda47b72 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Fri, 7 Jan 2022 17:48:40 +0000 Subject: [PATCH v5 1/1] Disallow external access to MaxBackends. Presently, MaxBackends is externally visible, but it may still be uninitialized in places where it would be convenient to use (e.g., _PG_init()). This change renames MaxBackends to MaxBackendsInternal, makes it static to postinit.c to disallow direct access, and introduces a MaxBackends macro for calling GetMaxBackends(). Separately, adjust the comments about needing to register background workers before initializing MaxBackends. Since 6bc8ef0b, InitializeMaxBackends() has used max_worker_processes instead of tallying up the number of registered background workers, so background worker registration is no longer a prerequisite. The ordering of this logic is still useful for allowing libraries to adjust GUCs, so the comments have been updated to mention that use- case. --- src/backend/postmaster/postmaster.c | 12 ++++------ src/backend/utils/init/globals.c | 4 ---- src/backend/utils/init/postinit.c | 47 ++++++++++++++++++++++++++++++------- src/include/miscadmin.h | 5 +++- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 328ecafa8c..6f64a9b4cb 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -1014,10 +1014,8 @@ PostmasterMain(int argc, char *argv[]) LocalProcessControlFile(false); /* - * Register the apply launcher. Since it registers a background worker, - * it needs to be called before InitializeMaxBackends(), and it's probably - * a good idea to call it before any modules had chance to take the - * background worker slots. + * Register the apply launcher. It's probably a good idea to call it before + * any modules had chance to take the background worker slots. */ ApplyLauncherRegister(); @@ -1038,8 +1036,8 @@ PostmasterMain(int argc, char *argv[]) #endif /* - * Now that loadable modules have had their chance to register background - * workers, calculate MaxBackends. + * Now that loadable modules have had their chance to alter any GUCs, + * calculate MaxBackends. */ InitializeMaxBackends(); @@ -6496,7 +6494,7 @@ restore_backend_variables(BackendParameters *param, Port *port) query_id_enabled = param->query_id_enabled; max_safe_fds = param->max_safe_fds; - MaxBackends = param->MaxBackends; + SetMaxBackends(param->MaxBackends); #ifdef WIN32 PostmasterHandle = param->PostmasterHandle; diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 381d9e548d..47884bbe85 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -128,15 +128,11 @@ int max_parallel_maintenance_workers = 2; /* * Primary determinants of sizes of shared-memory structures. - * - * MaxBackends is computed by PostmasterMain after modules have had a chance to - * register background workers. */ int NBuffers = 1000; int MaxConnections = 90; int max_worker_processes = 8; int max_parallel_workers = 8; -int MaxBackends = 0; int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ int VacuumCostPageMiss = 2; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 7292e51f7d..aeeb709a67 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -63,6 +63,9 @@ #include "utils/syscache.h" #include "utils/timeout.h" +static bool MaxBackendsInitialized = false; +static int MaxBackendsInternal = 0; + static HeapTuple GetDatabaseTuple(const char *dbname); static HeapTuple GetDatabaseTupleByOid(Oid dboid); static void PerformAuthentication(Port *port); @@ -476,9 +479,8 @@ pg_split_opts(char **argv, int *argcp, const char *optstr) /* * Initialize MaxBackends value from config options. * - * This must be called after modules have had the chance to register background - * workers in shared_preload_libraries, and before shared memory size is - * determined. + * This must be called after modules have had the chance to alter GUCs in + * shared_preload_libraries, and before shared memory size is determined. * * Note that in EXEC_BACKEND environment, the value is passed down from * postmaster to subprocesses via BackendParameters in SubPostmasterMain; only @@ -488,15 +490,42 @@ pg_split_opts(char **argv, int *argcp, const char *optstr) void 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; + SetMaxBackends(MaxConnections + autovacuum_max_workers + 1 + + max_worker_processes + max_wal_senders); +} - /* internal error because the values were all checked previously */ - if (MaxBackends > MAX_BACKENDS) +/* + * Retrieve the value for MaxBackends. + * + * If the value has not yet been initialized, this function will ERROR. + */ +int +GetMaxBackends(void) +{ + if (!MaxBackendsInitialized) + elog(ERROR, "MaxBackends not yet initialized"); + + return MaxBackendsInternal; +} + +/* + * Set the value for MaxBackends. + * + * If the value is already initialized or the value provided is not valid, this + * function will ERROR. + */ +void +SetMaxBackends(int max_backends) +{ + if (MaxBackendsInitialized) + elog(ERROR, "MaxBackends already initialized"); + + if (max_backends > MAX_BACKENDS) elog(ERROR, "too many backends configured"); + + MaxBackendsInternal = max_backends; + MaxBackendsInitialized = true; } /* diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 90a3016065..4860a9540a 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -172,7 +172,6 @@ extern PGDLLIMPORT char *DataDir; extern PGDLLIMPORT int data_directory_mode; extern PGDLLIMPORT int NBuffers; -extern PGDLLIMPORT int MaxBackends; extern PGDLLIMPORT int MaxConnections; extern PGDLLIMPORT int max_worker_processes; extern PGDLLIMPORT int max_parallel_workers; @@ -457,10 +456,14 @@ extern AuxProcType MyAuxProcType; /* in utils/init/postinit.c */ extern void pg_split_opts(char **argv, int *argcp, const char *optstr); extern void InitializeMaxBackends(void); +extern int GetMaxBackends(void); +extern void SetMaxBackends(int max_backends); extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username, Oid useroid, char *out_dbname, bool override_allow_connections); extern void BaseInit(void); +#define MaxBackends (GetMaxBackends()) + /* in utils/init/miscinit.c */ extern bool IgnoreSystemIndexes; extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress; -- 2.16.6 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make MaxBackends available in _PG_init @ 2022-01-07 20:20 Bossart, Nathan <[email protected]> parent: Bossart, Nathan <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Bossart, Nathan @ 2022-01-07 20:20 UTC (permalink / raw) To: Fujii Masao <[email protected]>; [email protected] <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]> On 1/7/22, 10:12 AM, "Bossart, Nathan" <[email protected]> wrote: > While that approach would provide a way to safely retrieve the value, > I think it would do little to prevent the issue in practice. If the > size of the patch is a concern, we could also convert MaxBackends into > a macro for calling GetMaxBackends(). This could also be a nice way > to avoid breaking extensions that are using it correctly while > triggering ERRORs for extensions that are using it incorrectly. I've > attached a new version of the patch that does it this way. v5 didn't work with EXEC_BACKEND. Here is a new revision. Nathan Attachments: [application/octet-stream] v6-0001-Disallow-external-access-to-MaxBackends.patch (7.2K, ../../[email protected]/2-v6-0001-Disallow-external-access-to-MaxBackends.patch) download | inline diff: From 615dfbd3352a235aa615098a13e3de33edb60c5a Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Fri, 7 Jan 2022 19:50:57 +0000 Subject: [PATCH v6 1/1] Disallow external access to MaxBackends. Presently, MaxBackends is externally visible, but it may still be uninitialized in places where it would be convenient to use (e.g., _PG_init()). This change renames MaxBackends to MaxBackendsInternal, makes it static to postinit.c to disallow direct access, and introduces a MaxBackends macro for calling GetMaxBackends(). Separately, adjust the comments about needing to register background workers before initializing MaxBackends. Since 6bc8ef0b, InitializeMaxBackends() has used max_worker_processes instead of tallying up the number of registered background workers, so background worker registration is no longer a prerequisite. The ordering of this logic is still useful for allowing libraries to adjust GUCs, so the comments have been updated to mention that use- case. --- src/backend/postmaster/postmaster.c | 16 ++++++------- src/backend/utils/init/globals.c | 4 ---- src/backend/utils/init/postinit.c | 47 ++++++++++++++++++++++++++++++------- src/include/miscadmin.h | 5 +++- 4 files changed, 49 insertions(+), 23 deletions(-) diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 328ecafa8c..1609e24114 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -525,7 +525,7 @@ typedef struct bool IsBinaryUpgrade; bool query_id_enabled; int max_safe_fds; - int MaxBackends; + int max_backends; #ifdef WIN32 HANDLE PostmasterHandle; HANDLE initial_signal_pipe; @@ -1014,10 +1014,8 @@ PostmasterMain(int argc, char *argv[]) LocalProcessControlFile(false); /* - * Register the apply launcher. Since it registers a background worker, - * it needs to be called before InitializeMaxBackends(), and it's probably - * a good idea to call it before any modules had chance to take the - * background worker slots. + * Register the apply launcher. It's probably a good idea to call it before + * any modules had chance to take the background worker slots. */ ApplyLauncherRegister(); @@ -1038,8 +1036,8 @@ PostmasterMain(int argc, char *argv[]) #endif /* - * Now that loadable modules have had their chance to register background - * workers, calculate MaxBackends. + * Now that loadable modules have had their chance to alter any GUCs, + * calculate MaxBackends. */ InitializeMaxBackends(); @@ -6262,7 +6260,7 @@ save_backend_variables(BackendParameters *param, Port *port, param->query_id_enabled = query_id_enabled; param->max_safe_fds = max_safe_fds; - param->MaxBackends = MaxBackends; + param->max_backends = MaxBackends; #ifdef WIN32 param->PostmasterHandle = PostmasterHandle; @@ -6496,7 +6494,7 @@ restore_backend_variables(BackendParameters *param, Port *port) query_id_enabled = param->query_id_enabled; max_safe_fds = param->max_safe_fds; - MaxBackends = param->MaxBackends; + SetMaxBackends(param->max_backends); #ifdef WIN32 PostmasterHandle = param->PostmasterHandle; diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 381d9e548d..47884bbe85 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -128,15 +128,11 @@ int max_parallel_maintenance_workers = 2; /* * Primary determinants of sizes of shared-memory structures. - * - * MaxBackends is computed by PostmasterMain after modules have had a chance to - * register background workers. */ int NBuffers = 1000; int MaxConnections = 90; int max_worker_processes = 8; int max_parallel_workers = 8; -int MaxBackends = 0; int VacuumCostPageHit = 1; /* GUC parameters for vacuum */ int VacuumCostPageMiss = 2; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 7292e51f7d..aeeb709a67 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -63,6 +63,9 @@ #include "utils/syscache.h" #include "utils/timeout.h" +static bool MaxBackendsInitialized = false; +static int MaxBackendsInternal = 0; + static HeapTuple GetDatabaseTuple(const char *dbname); static HeapTuple GetDatabaseTupleByOid(Oid dboid); static void PerformAuthentication(Port *port); @@ -476,9 +479,8 @@ pg_split_opts(char **argv, int *argcp, const char *optstr) /* * Initialize MaxBackends value from config options. * - * This must be called after modules have had the chance to register background - * workers in shared_preload_libraries, and before shared memory size is - * determined. + * This must be called after modules have had the chance to alter GUCs in + * shared_preload_libraries, and before shared memory size is determined. * * Note that in EXEC_BACKEND environment, the value is passed down from * postmaster to subprocesses via BackendParameters in SubPostmasterMain; only @@ -488,15 +490,42 @@ pg_split_opts(char **argv, int *argcp, const char *optstr) void 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; + SetMaxBackends(MaxConnections + autovacuum_max_workers + 1 + + max_worker_processes + max_wal_senders); +} - /* internal error because the values were all checked previously */ - if (MaxBackends > MAX_BACKENDS) +/* + * Retrieve the value for MaxBackends. + * + * If the value has not yet been initialized, this function will ERROR. + */ +int +GetMaxBackends(void) +{ + if (!MaxBackendsInitialized) + elog(ERROR, "MaxBackends not yet initialized"); + + return MaxBackendsInternal; +} + +/* + * Set the value for MaxBackends. + * + * If the value is already initialized or the value provided is not valid, this + * function will ERROR. + */ +void +SetMaxBackends(int max_backends) +{ + if (MaxBackendsInitialized) + elog(ERROR, "MaxBackends already initialized"); + + if (max_backends > MAX_BACKENDS) elog(ERROR, "too many backends configured"); + + MaxBackendsInternal = max_backends; + MaxBackendsInitialized = true; } /* diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 90a3016065..4860a9540a 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -172,7 +172,6 @@ extern PGDLLIMPORT char *DataDir; extern PGDLLIMPORT int data_directory_mode; extern PGDLLIMPORT int NBuffers; -extern PGDLLIMPORT int MaxBackends; extern PGDLLIMPORT int MaxConnections; extern PGDLLIMPORT int max_worker_processes; extern PGDLLIMPORT int max_parallel_workers; @@ -457,10 +456,14 @@ extern AuxProcType MyAuxProcType; /* in utils/init/postinit.c */ extern void pg_split_opts(char **argv, int *argcp, const char *optstr); extern void InitializeMaxBackends(void); +extern int GetMaxBackends(void); +extern void SetMaxBackends(int max_backends); extern void InitPostgres(const char *in_dbname, Oid dboid, const char *username, Oid useroid, char *out_dbname, bool override_allow_connections); extern void BaseInit(void); +#define MaxBackends (GetMaxBackends()) + /* in utils/init/miscinit.c */ extern bool IgnoreSystemIndexes; extern PGDLLIMPORT bool process_shared_preload_libraries_in_progress; -- 2.16.6 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make MaxBackends available in _PG_init @ 2022-01-07 20:26 Robert Haas <[email protected]> parent: Bossart, Nathan <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Robert Haas @ 2022-01-07 20:26 UTC (permalink / raw) To: Bossart, Nathan <[email protected]>; +Cc: Fujii Masao <[email protected]>; [email protected] <[email protected]>; Andres Freund <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]> On Fri, Jan 7, 2022 at 1:09 PM Bossart, Nathan <[email protected]> wrote: > I wouldn't be surprised to learn of other cases, but I've only > encountered this specific issue with MaxBackends. I think MaxBackends > is notable because it is more likely to be used by preloaded libraries > but is intentionally initialized after loading them. As noted in > an earlier message on this thread [0], using MaxBackends in a call to > RequestAddinShmemSpace() in _PG_init() may not reliably cause > problems, too. Yes, I think MaxBackends is a particularly severe case. I've seen this problem with that GUC multiple times, and never with any other one. > > It seems overkill to remove "extern" from MaxBackends and replace MaxBackends with GetMaxBackends() in the existing PostgreSQL codes. I'm not sure how much it's actually worth doing that. Instead, isn't it enough to just add the comment like "Use GetMaxBackends() if you want to treat the lookup for uninitialized MaxBackends as an error" in the definition of MaxBackends? > > While that approach would provide a way to safely retrieve the value, > I think it would do little to prevent the issue in practice. If the > size of the patch is a concern, we could also convert MaxBackends into > a macro for calling GetMaxBackends(). This could also be a nice way > to avoid breaking extensions that are using it correctly while > triggering ERRORs for extensions that are using it incorrectly. I've > attached a new version of the patch that does it this way. That's too magical for my taste. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: make MaxBackends available in _PG_init @ 2022-01-10 19:01 Bossart, Nathan <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bossart, Nathan @ 2022-01-10 19:01 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Fujii Masao <[email protected]>; [email protected] <[email protected]>; Andres Freund <[email protected]>; Bharath Rupireddy <[email protected]>; Greg Sabino Mullane <[email protected]>; Tom Lane <[email protected]>; [email protected] <[email protected]> On 1/7/22, 12:27 PM, "Robert Haas" <[email protected]> wrote: > On Fri, Jan 7, 2022 at 1:09 PM Bossart, Nathan <[email protected]> wrote: >> While that approach would provide a way to safely retrieve the value, >> I think it would do little to prevent the issue in practice. If the >> size of the patch is a concern, we could also convert MaxBackends into >> a macro for calling GetMaxBackends(). This could also be a nice way >> to avoid breaking extensions that are using it correctly while >> triggering ERRORs for extensions that are using it incorrectly. I've >> attached a new version of the patch that does it this way. > > That's too magical for my taste. Fair point. v4 [0] is the less magical version. Nathan [0] https://postgr.es/m/attachment/125445/v4-0001-Disallow-external-access-to-MaxBackends.patch ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-01-10 19:01 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-01 19:46 [PATCH v13 06/18] More refactoring Justin Pryzby <[email protected]> 2022-01-07 18:09 Re: make MaxBackends available in _PG_init Bossart, Nathan <[email protected]> 2022-01-07 20:20 ` Re: make MaxBackends available in _PG_init Bossart, Nathan <[email protected]> 2022-01-07 20:26 ` Re: make MaxBackends available in _PG_init Robert Haas <[email protected]> 2022-01-10 19:01 ` Re: make MaxBackends available in _PG_init Bossart, Nathan <[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