Modernizing our GUC infrastructure - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Modernizing our GUC infrastructure |
Date | |
Msg-id | 2982579.1662416866@sss.pgh.pa.us Whole thread Raw |
Responses |
Re: Modernizing our GUC infrastructure
Re: Modernizing our GUC infrastructure Re: Modernizing our GUC infrastructure Re: Modernizing our GUC infrastructure |
List | pgsql-hackers |
Attached is a patch series that attempts to modernize our GUC infrastructure, in particular removing the performance bottlenecks it has when there are lots of GUC variables. I wrote this because I am starting to question the schema-variables patch [1] --- that's getting to be quite a large patch and I grow less and less sure that it's solving a problem our users want solved. I think what people actually want is better support of the existing mechanism for ad-hoc session variables, namely abusing custom GUCs for that purpose. One of the big reasons we have been resistant to formally supporting that is fear of the poor performance guc.c would have with lots of variables. But we already have quite a lot of them: regression=# select count(*) from pg_settings; count ------- 353 (1 row) and more are getting added all the time. I think this patch series could likely be justified just in terms of positive effect on core performance, never mind user-added GUCs. 0001 and 0002 below are concerned with converting guc.c to store its data in a dedicated memory context (GUCMemoryContext) instead of using raw malloc(). This is not directly a performance improvement, and I'm prepared to drop the idea if there's a lot of pushback, but I think it would be a good thing to do. The only hard reason for using malloc() there was the lack of ability to avoid throwing elog(ERROR) on out-of-memory in palloc(). But mcxt.c grew that ability years ago. Switching to a dedicated context would greatly improve visibility and accountability of GUC-related allocations. Also, the 0003 patch will switch guc.c to relying on a palloc-based hashtable, and it seems a bit silly to have part of the data structure in palloc and part in malloc. However 0002 is a bit invasive, in that it forces code changes in GUC check callbacks, if they want to reallocate the new value or create an "extra" data structure. My feeling is that not enough external modules use those facilities for this to pose a big problem. However, the ones that are subject to it will have a non-fun time tracking down why their code is crashing. (The recent context-header changes mean that you get a very obscure failure when trying to pfree() a malloc'd chunk -- for me, that typically ends in an assertion failure in generation.c. Can we make that less confusing?) 0003 replaces guc.c's bsearch-a-sorted-array lookup infrastructure with a dynahash hash table. (I also looked at simplehash, but it has no support for not elog'ing on OOM, and it increases the size of guc.o by 10KB or so.) This fixes the worse-than-O(N^2) time needed to create N new GUCs, as in do $$ begin for i in 1..10000 loop perform set_config('foo.bar' || i::text, i::text, false); end loop; end $$; On my machine, this takes about 4700 ms in HEAD, versus 23 ms with this patch. However, the places that were linearly scanning the array now need to use hash_seq_search, so some other things like transaction shutdown (AtEOXact_GUC) get slower. To address that, 0004 adds some auxiliary lists that link together just the variables that are interesting for specific purposes. This is helpful even without considering the possibility of a lot of user-added GUCs: in a typical session, for example, not many of those 353 GUCs have non-default values, and even fewer get modified in any one transaction (typically, anyway). As an example of the speedup from 0004, these DO loops: create or replace function func_with_set(int) returns int strict immutable language plpgsql as $$ begin return $1; end $$ set enable_seqscan = false; do $$ begin for i in 1..100000 loop perform func_with_set(i); end loop; end $$; do $$ begin for i in 1..100000 loop begin perform func_with_set(i); exception when others then raise; end; end loop; end $$; take about 260 and 320 ms respectively for me, in HEAD with just the stock set of variables. But after creating 10000 GUCs with the previous DO loop, they're up to about 3200 ms. 0004 brings that back down to being indistinguishable from the speed with few GUCs. So I think this is good cleanup in its own right, plus it removes one major objection to considering user-defined GUCs as a supported feature. regards, tom lane [1] https://www.postgresql.org/message-id/flat/CAFj8pRD053CY_N4%3D6SvPe7ke6xPbh%3DK50LUAOwjC3jm8Me9Obg%40mail.gmail.com commit ecaef8ecf5705b9638f81d2dfb300b57589fbb8a Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Mon Sep 5 16:14:41 2022 -0400 Preliminary improvements in memory-context infrastructure. We lack a version of repalloc() that supports MCXT_ALLOC_NO_OOM semantics, so invent repalloc_extended(). repalloc_huge() becomes a legacy wrapper for that. Also, fix dynahash.c so that it can support HASH_ENTER_NULL requests when using the default palloc-based allocator. The only reason it was like that was the lack of the MCXT_ALLOC_NO_OOM option when that code was written, ages ago. While here, simplify a few overcomplicated tests in mcxt.c. diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 3babde8d70..4f62958883 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -289,7 +289,8 @@ static void * DynaHashAlloc(Size size) { Assert(MemoryContextIsValid(CurrentDynaHashCxt)); - return MemoryContextAlloc(CurrentDynaHashCxt, size); + return MemoryContextAllocExtended(CurrentDynaHashCxt, size, + MCXT_ALLOC_NO_OOM); } @@ -939,9 +940,7 @@ calc_bucket(HASHHDR *hctl, uint32 hash_val) * * HASH_ENTER will normally ereport a generic "out of memory" error if * it is unable to create a new entry. The HASH_ENTER_NULL operation is - * the same except it will return NULL if out of memory. Note that - * HASH_ENTER_NULL cannot be used with the default palloc-based allocator, - * since palloc internally ereports on out-of-memory. + * the same except it will return NULL if out of memory. * * If foundPtr isn't NULL, then *foundPtr is set true if we found an * existing entry in the table, false otherwise. This is needed in the @@ -1084,12 +1083,8 @@ hash_search_with_hash_value(HTAB *hashp, } return NULL; - case HASH_ENTER_NULL: - /* ENTER_NULL does not work with palloc-based allocator */ - Assert(hashp->alloc != DynaHashAlloc); - /* FALL THRU */ - case HASH_ENTER: + case HASH_ENTER_NULL: /* Return existing element if found, else create one */ if (currBucket != NULL) return (void *) ELEMENTKEY(currBucket); diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c index 64bcc7ef32..5618944e4c 100644 --- a/src/backend/utils/mmgr/mcxt.c +++ b/src/backend/utils/mmgr/mcxt.c @@ -1044,8 +1044,8 @@ MemoryContextAllocExtended(MemoryContext context, Size size, int flags) AssertArg(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - if (((flags & MCXT_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) || - ((flags & MCXT_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size))) + if (!((flags & MCXT_ALLOC_HUGE) != 0 ? AllocHugeSizeIsValid(size) : + AllocSizeIsValid(size))) elog(ERROR, "invalid memory alloc request size %zu", size); context->isReset = false; @@ -1199,8 +1199,8 @@ palloc_extended(Size size, int flags) AssertArg(MemoryContextIsValid(context)); AssertNotInCriticalSection(context); - if (((flags & MCXT_ALLOC_HUGE) != 0 && !AllocHugeSizeIsValid(size)) || - ((flags & MCXT_ALLOC_HUGE) == 0 && !AllocSizeIsValid(size))) + if (!((flags & MCXT_ALLOC_HUGE) != 0 ? AllocHugeSizeIsValid(size) : + AllocSizeIsValid(size))) elog(ERROR, "invalid memory alloc request size %zu", size); context->isReset = false; @@ -1281,6 +1281,50 @@ repalloc(void *pointer, Size size) return ret; } +/* + * repalloc_extended + * Adjust the size of a previously allocated chunk, + * with HUGE and NO_OOM options. + */ +void * +repalloc_extended(void *pointer, Size size, int flags) +{ +#if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND) + MemoryContext context = GetMemoryChunkContext(pointer); +#endif + void *ret; + + if (!((flags & MCXT_ALLOC_HUGE) != 0 ? AllocHugeSizeIsValid(size) : + AllocSizeIsValid(size))) + elog(ERROR, "invalid memory alloc request size %zu", size); + + AssertNotInCriticalSection(context); + + /* isReset must be false already */ + Assert(!context->isReset); + + ret = MCXT_METHOD(pointer, realloc) (pointer, size); + if (unlikely(ret == NULL)) + { + if ((flags & MCXT_ALLOC_NO_OOM) == 0) + { + MemoryContext cxt = GetMemoryChunkContext(pointer); + + MemoryContextStats(TopMemoryContext); + ereport(ERROR, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"), + errdetail("Failed on request of size %zu in memory context \"%s\".", + size, cxt->name))); + } + return NULL; + } + + VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size); + + return ret; +} + /* * MemoryContextAllocHuge * Allocate (possibly-expansive) space within the specified context. @@ -1324,35 +1368,8 @@ MemoryContextAllocHuge(MemoryContext context, Size size) void * repalloc_huge(void *pointer, Size size) { -#if defined(USE_ASSERT_CHECKING) || defined(USE_VALGRIND) - MemoryContext context = GetMemoryChunkContext(pointer); -#endif - void *ret; - - if (!AllocHugeSizeIsValid(size)) - elog(ERROR, "invalid memory alloc request size %zu", size); - - AssertNotInCriticalSection(context); - - /* isReset must be false already */ - Assert(!context->isReset); - - ret = MCXT_METHOD(pointer, realloc) (pointer, size); - if (unlikely(ret == NULL)) - { - MemoryContext cxt = GetMemoryChunkContext(pointer); - - MemoryContextStats(TopMemoryContext); - ereport(ERROR, - (errcode(ERRCODE_OUT_OF_MEMORY), - errmsg("out of memory"), - errdetail("Failed on request of size %zu in memory context \"%s\".", - size, cxt->name))); - } - - VALGRIND_MEMPOOL_CHANGE(context, pointer, ret, size); - - return ret; + /* this one seems not worth its own implementation */ + return repalloc_extended(pointer, size, MCXT_ALLOC_HUGE); } /* diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h index 332575f518..f31ab3d7af 100644 --- a/src/include/utils/palloc.h +++ b/src/include/utils/palloc.h @@ -78,6 +78,8 @@ extern void *palloc(Size size); extern void *palloc0(Size size); extern void *palloc_extended(Size size, int flags); extern pg_nodiscard void *repalloc(void *pointer, Size size); +extern pg_nodiscard void *repalloc_extended(void *pointer, + Size size, int flags); extern void pfree(void *pointer); /* commit 2c4abdf5280e13d243d9c0953bc59a7cb32d4aaa Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Mon Sep 5 16:28:24 2022 -0400 Store GUC data in a memory context, not with malloc(). The only real argument for using malloc directly was that we needed the ability to not throw error on OOM; but mcxt.c grew that feature awhile ago. Keeping the data in a memory context improves accountability and debuggability --- for example, without this it's almost impossible to detect memory leaks in the GUC code with anything less costly than valgrind. Moreover, the next patch in this series will add a hash table for GUC lookup, and it'd be pretty silly to be using palloc-dependent hash facilities along with malloc storage of the underlying data. This is a bit invasive though, in particular causing an API break for GUC check hooks that want to modify the GUC's value or use an "extra" data structure. My guess is that not very many extensions will be affected by that, but perhaps I'm wrong. One note is that this changes ParseLongOption() to return short-lived palloc'd not malloc'd data. There wasn't any caller for which the latter was better. diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c index 58752368e7..ee54723c8d 100644 --- a/src/backend/bootstrap/bootstrap.c +++ b/src/backend/bootstrap/bootstrap.c @@ -287,8 +287,8 @@ BootstrapModeMain(int argc, char *argv[], bool check_only) } SetConfigOption(name, value, PGC_POSTMASTER, PGC_S_ARGV); - free(name); - free(value); + pfree(name); + pfree(value); break; } default: diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index f260b484fc..3ac99afc54 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -1290,8 +1290,8 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source) } /* Now prepare an "extra" struct for assign_temp_tablespaces */ - myextra = malloc(offsetof(temp_tablespaces_extra, tblSpcs) + - numSpcs * sizeof(Oid)); + myextra = guc_malloc(LOG, offsetof(temp_tablespaces_extra, tblSpcs) + + numSpcs * sizeof(Oid)); if (!myextra) return false; myextra->numSpcs = numSpcs; diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index e5ddcda0b4..a12eb87ca5 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -140,7 +140,7 @@ check_datestyle(char **newval, void **extra, GucSource source) char *subval; void *subextra = NULL; - subval = strdup(GetConfigOptionResetString("datestyle")); + subval = guc_strdup(LOG, GetConfigOptionResetString("datestyle")); if (!subval) { ok = false; @@ -148,7 +148,7 @@ check_datestyle(char **newval, void **extra, GucSource source) } if (!check_datestyle(&subval, &subextra, source)) { - free(subval); + guc_free(subval); ok = false; break; } @@ -157,8 +157,8 @@ check_datestyle(char **newval, void **extra, GucSource source) newDateStyle = myextra[0]; if (!have_order) newDateOrder = myextra[1]; - free(subval); - free(subextra); + guc_free(subval); + guc_free(subextra); } else { @@ -179,9 +179,9 @@ check_datestyle(char **newval, void **extra, GucSource source) } /* - * Prepare the canonical string to return. GUC wants it malloc'd. + * Prepare the canonical string to return. GUC wants it guc_malloc'd. */ - result = (char *) malloc(32); + result = (char *) guc_malloc(LOG, 32); if (!result) return false; @@ -213,13 +213,13 @@ check_datestyle(char **newval, void **extra, GucSource source) break; } - free(*newval); + guc_free(*newval); *newval = result; /* * Set up the "extra" struct actually used by assign_datestyle. */ - myextra = (int *) malloc(2 * sizeof(int)); + myextra = (int *) guc_malloc(LOG, 2 * sizeof(int)); if (!myextra) return false; myextra[0] = newDateStyle; @@ -358,7 +358,7 @@ check_timezone(char **newval, void **extra, GucSource source) /* * Pass back data for assign_timezone to use */ - *extra = malloc(sizeof(pg_tz *)); + *extra = guc_malloc(LOG, sizeof(pg_tz *)); if (!*extra) return false; *((pg_tz **) *extra) = new_tz; @@ -431,7 +431,7 @@ check_log_timezone(char **newval, void **extra, GucSource source) /* * Pass back data for assign_log_timezone to use */ - *extra = malloc(sizeof(pg_tz *)); + *extra = guc_malloc(LOG, sizeof(pg_tz *)); if (!*extra) return false; *((pg_tz **) *extra) = new_tz; @@ -589,7 +589,7 @@ check_transaction_deferrable(bool *newval, void **extra, GucSource source) bool check_random_seed(double *newval, void **extra, GucSource source) { - *extra = malloc(sizeof(int)); + *extra = guc_malloc(LOG, sizeof(int)); if (!*extra) return false; /* Arm the assign only if source of value is an interactive SET */ @@ -677,8 +677,8 @@ check_client_encoding(char **newval, void **extra, GucSource source) if (strcmp(*newval, canonical_name) != 0 && strcmp(*newval, "UNICODE") != 0) { - free(*newval); - *newval = strdup(canonical_name); + guc_free(*newval); + *newval = guc_strdup(LOG, canonical_name); if (!*newval) return false; } @@ -686,7 +686,7 @@ check_client_encoding(char **newval, void **extra, GucSource source) /* * Save the encoding's ID in *extra, for use by assign_client_encoding. */ - *extra = malloc(sizeof(int)); + *extra = guc_malloc(LOG, sizeof(int)); if (!*extra) return false; *((int *) *extra) = encoding; @@ -789,7 +789,7 @@ check_session_authorization(char **newval, void **extra, GucSource source) ReleaseSysCache(roleTup); /* Set up "extra" struct for assign_session_authorization to use */ - myextra = (role_auth_extra *) malloc(sizeof(role_auth_extra)); + myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra)); if (!myextra) return false; myextra->roleid = roleid; @@ -899,7 +899,7 @@ check_role(char **newval, void **extra, GucSource source) } /* Set up "extra" struct for assign_role to use */ - myextra = (role_auth_extra *) malloc(sizeof(role_auth_extra)); + myextra = (role_auth_extra *) guc_malloc(LOG, sizeof(role_auth_extra)); if (!myextra) return false; myextra->roleid = roleid; diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 1664fcee2a..7be983044b 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -849,8 +849,8 @@ PostmasterMain(int argc, char *argv[]) } SetConfigOption(name, value, PGC_POSTMASTER, PGC_S_ARGV); - free(name); - free(value); + pfree(name); + pfree(value); break; } diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index ce163b99e9..14c464fbbf 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -1053,9 +1053,9 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source) return false; } - /* GUC extra value must be malloc'd, not palloc'd */ + /* GUC extra value must be guc_malloc'd, not palloc'd */ pconf = (SyncRepConfigData *) - malloc(syncrep_parse_result->config_size); + guc_malloc(LOG, syncrep_parse_result->config_size); if (pconf == NULL) return false; memcpy(pconf, syncrep_parse_result, syncrep_parse_result->config_size); diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 7bec4e4ff5..2e951edd18 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3817,8 +3817,8 @@ process_postgres_switches(int argc, char *argv[], GucContext ctx, optarg))); } SetConfigOption(name, value, ctx, gucsource); - free(name); - free(value); + pfree(name); + pfree(value); break; } diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index 43fff50d49..98a90a81c0 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -29,6 +29,7 @@ #include "utils/builtins.h" #include "utils/date.h" #include "utils/datetime.h" +#include "utils/guc.h" #include "utils/memutils.h" #include "utils/tzparser.h" @@ -4782,8 +4783,8 @@ TemporalSimplify(int32 max_precis, Node *node) * to create the final array of timezone tokens. The argument array * is already sorted in name order. * - * The result is a TimeZoneAbbrevTable (which must be a single malloc'd chunk) - * or NULL on malloc failure. No other error conditions are defined. + * The result is a TimeZoneAbbrevTable (which must be a single guc_malloc'd + * chunk) or NULL on alloc failure. No other error conditions are defined. */ TimeZoneAbbrevTable * ConvertTimeZoneAbbrevs(struct tzEntry *abbrevs, int n) @@ -4812,7 +4813,7 @@ ConvertTimeZoneAbbrevs(struct tzEntry *abbrevs, int n) } /* Alloc the result ... */ - tbl = malloc(tbl_size); + tbl = guc_malloc(LOG, tbl_size); if (!tbl) return NULL; diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c index 24808dfbb1..53842d4fe5 100644 --- a/src/backend/utils/cache/ts_cache.c +++ b/src/backend/utils/cache/ts_cache.c @@ -632,9 +632,9 @@ check_TSCurrentConfig(char **newval, void **extra, GucSource source) ReleaseSysCache(tuple); - /* GUC wants it malloc'd not palloc'd */ - free(*newval); - *newval = strdup(buf); + /* GUC wants it guc_malloc'd not palloc'd */ + guc_free(*newval); + *newval = guc_strdup(LOG, buf); pfree(buf); if (!*newval) return false; diff --git a/src/backend/utils/misc/README b/src/backend/utils/misc/README index 6e294386f7..85d97d29b6 100644 --- a/src/backend/utils/misc/README +++ b/src/backend/utils/misc/README @@ -51,13 +51,13 @@ out-of-memory. This might be used for example to canonicalize the spelling of a string value, round off a buffer size to the nearest supported value, or replace a special value such as "-1" with a computed default value. If the -function wishes to replace a string value, it must malloc (not palloc) -the replacement value, and be sure to free() the previous value. +function wishes to replace a string value, it must guc_malloc (not palloc) +the replacement value, and be sure to guc_free() the previous value. * Derived information, such as the role OID represented by a user name, -can be stored for use by the assign hook. To do this, malloc (not palloc) +can be stored for use by the assign hook. To do this, guc_malloc (not palloc) storage space for the information, and return its address at *extra. -guc.c will automatically free() this space when the associated GUC setting +guc.c will automatically guc_free() this space when the associated GUC setting is no longer of interest. *extra is initialized to NULL before call, so it can be ignored if not needed. @@ -255,10 +255,9 @@ maintained by GUC. GUC Memory Handling ------------------- -String variable values are allocated with malloc/strdup, not with the -palloc/pstrdup mechanisms. We would need to keep them in a permanent -context anyway, and malloc gives us more control over handling -out-of-memory failures. +String variable values are allocated with guc_malloc or guc_strdup, +which ensure that the values are kept in a long-lived context, and provide +more control over handling out-of-memory failures than bare palloc. We allow a string variable's actual value, reset_val, boot_val, and stacked values to point at the same storage. This makes it slightly harder to free diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index c336698ad5..e123ccb629 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5136,6 +5136,9 @@ static const char *const map_old_guc_names[] = { }; +/* Memory context holding all GUC-related data */ +static MemoryContext GUCMemoryContext; + /* * Actual lookup of variables is done through this single, sorted array. */ @@ -5531,54 +5534,69 @@ bail_out: return head; } + /* - * Some infrastructure for checking malloc/strdup/realloc calls + * Some infrastructure for GUC-related memory allocation + * + * These functions are generally modeled on libc's malloc/realloc/etc, + * but any OOM issue is reported at the specified elevel. + * (Thus, control returns only if that's less than ERROR.) */ -static void * +void * guc_malloc(int elevel, size_t size) { void *data; - /* Avoid unportable behavior of malloc(0) */ - if (size == 0) - size = 1; - data = malloc(size); - if (data == NULL) + data = MemoryContextAllocExtended(GUCMemoryContext, size, + MCXT_ALLOC_NO_OOM); + if (unlikely(data == NULL)) ereport(elevel, (errcode(ERRCODE_OUT_OF_MEMORY), errmsg("out of memory"))); return data; } -static void * +void * guc_realloc(int elevel, void *old, size_t size) { void *data; - /* Avoid unportable behavior of realloc(NULL, 0) */ - if (old == NULL && size == 0) - size = 1; - data = realloc(old, size); - if (data == NULL) + if (old != NULL) + data = repalloc_extended(old, size, + MCXT_ALLOC_NO_OOM); + else + data = MemoryContextAllocExtended(GUCMemoryContext, size, + MCXT_ALLOC_NO_OOM); + if (unlikely(data == NULL)) ereport(elevel, (errcode(ERRCODE_OUT_OF_MEMORY), errmsg("out of memory"))); return data; } -static char * +char * guc_strdup(int elevel, const char *src) { char *data; + size_t len = strlen(src) + 1; - data = strdup(src); - if (data == NULL) - ereport(elevel, - (errcode(ERRCODE_OUT_OF_MEMORY), - errmsg("out of memory"))); + data = guc_malloc(elevel, len); + if (likely(data != NULL)) + memcpy(data, src, len); return data; } +void +guc_free(void *ptr) +{ + /* + * Historically, GUC-related code has relied heavily on the ability to do + * free(NULL), so we allow that here even though pfree() doesn't. + */ + if (ptr != NULL) + pfree(ptr); +} + /* * Detect whether strval is referenced anywhere in a GUC string item @@ -5616,7 +5634,7 @@ set_string_field(struct config_string *conf, char **field, char *newval) /* Free old value if it's not NULL and isn't referenced anymore */ if (oldval && !string_field_used(conf, oldval)) - free(oldval); + guc_free(oldval); } /* @@ -5677,7 +5695,7 @@ set_extra_field(struct config_generic *gconf, void **field, void *newval) /* Free old value if it's not NULL and isn't referenced anymore */ if (oldval && !extra_field_used(gconf, oldval)) - free(oldval); + guc_free(oldval); } /* @@ -5685,7 +5703,7 @@ set_extra_field(struct config_generic *gconf, void **field, void *newval) * The "extra" field associated with the active value is copied, too. * * NB: be sure stringval and extra fields of a new stack entry are - * initialized to NULL before this is used, else we'll try to free() them. + * initialized to NULL before this is used, else we'll try to guc_free() them. */ static void set_stack_value(struct config_generic *gconf, config_var_value *val) @@ -5753,9 +5771,9 @@ get_guc_variables(void) /* - * Build the sorted array. This is split out so that it could be - * re-executed after startup (e.g., we could allow loadable modules to - * add vars, and then we'd need to re-sort). + * Build the sorted array. This is split out so that help_config.c can + * extract all the variables without running all of InitializeGUCOptions. + * It's not meant for use anyplace else. */ void build_guc_variables(void) @@ -5765,6 +5783,17 @@ build_guc_variables(void) struct config_generic **guc_vars; int i; + /* + * Create the memory context that will hold all GUC-related data. + */ + Assert(GUCMemoryContext == NULL); + GUCMemoryContext = AllocSetContextCreate(TopMemoryContext, + "GUCMemoryContext", + ALLOCSET_DEFAULT_SIZES); + + /* + * Count all the built-in variables, and set their vartypes correctly. + */ for (i = 0; ConfigureNamesBool[i].gen.name; i++) { struct config_bool *conf = &ConfigureNamesBool[i]; @@ -5831,7 +5860,7 @@ build_guc_variables(void) for (i = 0; ConfigureNamesEnum[i].gen.name; i++) guc_vars[num_vars++] = &ConfigureNamesEnum[i].gen; - free(guc_variables); + guc_free(guc_variables); guc_variables = guc_vars; num_guc_variables = num_vars; size_guc_variables = size_vars; @@ -5937,7 +5966,7 @@ add_placeholder_variable(const char *name, int elevel) gen->name = guc_strdup(elevel, name); if (gen->name == NULL) { - free(var); + guc_free(var); return NULL; } @@ -5956,8 +5985,8 @@ add_placeholder_variable(const char *name, int elevel) if (!add_guc_variable((struct config_generic *) var, elevel)) { - free(unconstify(char *, gen->name)); - free(var); + guc_free(unconstify(char *, gen->name)); + guc_free(var); return NULL; } @@ -6191,7 +6220,7 @@ InitializeGUCOptions(void) pg_timezone_initialize(); /* - * Build sorted array of all GUC variables. + * Create GUCMemoryContext and build sorted array of all GUC variables. */ build_guc_variables(); @@ -6447,6 +6476,7 @@ SelectConfigFiles(const char *userDoption, const char *progname) { char *configdir; char *fname; + bool fname_is_malloced; struct stat stat_buf; /* configdir is -D option, or $PGDATA if no -D */ @@ -6473,12 +6503,16 @@ SelectConfigFiles(const char *userDoption, const char *progname) * the same way by future backends. */ if (ConfigFileName) + { fname = make_absolute_path(ConfigFileName); + fname_is_malloced = true; + } else if (configdir) { fname = guc_malloc(FATAL, strlen(configdir) + strlen(CONFIG_FILENAME) + 2); sprintf(fname, "%s/%s", configdir, CONFIG_FILENAME); + fname_is_malloced = false; } else { @@ -6494,7 +6528,11 @@ SelectConfigFiles(const char *userDoption, const char *progname) * it can't be overridden later. */ SetConfigOption("config_file", fname, PGC_POSTMASTER, PGC_S_OVERRIDE); - free(fname); + + if (fname_is_malloced) + free(fname); + else + guc_free(fname); /* * Now read the config file for the first time. @@ -6566,12 +6604,16 @@ SelectConfigFiles(const char *userDoption, const char *progname) * Figure out where pg_hba.conf is, and make sure the path is absolute. */ if (HbaFileName) + { fname = make_absolute_path(HbaFileName); + fname_is_malloced = true; + } else if (configdir) { fname = guc_malloc(FATAL, strlen(configdir) + strlen(HBA_FILENAME) + 2); sprintf(fname, "%s/%s", configdir, HBA_FILENAME); + fname_is_malloced = false; } else { @@ -6583,18 +6625,26 @@ SelectConfigFiles(const char *userDoption, const char *progname) return false; } SetConfigOption("hba_file", fname, PGC_POSTMASTER, PGC_S_OVERRIDE); - free(fname); + + if (fname_is_malloced) + free(fname); + else + guc_free(fname); /* * Likewise for pg_ident.conf. */ if (IdentFileName) + { fname = make_absolute_path(IdentFileName); + fname_is_malloced = true; + } else if (configdir) { fname = guc_malloc(FATAL, strlen(configdir) + strlen(IDENT_FILENAME) + 2); sprintf(fname, "%s/%s", configdir, IDENT_FILENAME); + fname_is_malloced = false; } else { @@ -6606,7 +6656,11 @@ SelectConfigFiles(const char *userDoption, const char *progname) return false; } SetConfigOption("ident_file", fname, PGC_POSTMASTER, PGC_S_OVERRIDE); - free(fname); + + if (fname_is_malloced) + free(fname); + else + guc_free(fname); free(configdir); @@ -7234,12 +7288,12 @@ ReportGUCOption(struct config_generic *record) pq_endmessage(&msgbuf); /* - * We need a long-lifespan copy. If strdup() fails due to OOM, we'll - * set last_reported to NULL and thereby possibly make a duplicate - * report later. + * We need a long-lifespan copy. If guc_strdup() fails due to OOM, + * we'll set last_reported to NULL and thereby possibly make a + * duplicate report later. */ - free(record->last_reported); - record->last_reported = strdup(val); + guc_free(record->last_reported); + record->last_reported = guc_strdup(LOG, val); } pfree(val); @@ -7838,7 +7892,7 @@ parse_and_validate_value(struct config_generic *record, if (!call_string_check_hook(conf, &newval->stringval, newextra, source, elevel)) { - free(newval->stringval); + guc_free(newval->stringval); newval->stringval = NULL; return false; } @@ -8253,7 +8307,7 @@ set_config_option_ext(const char *name, const char *value, { /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); if (*conf->variable != newval) { @@ -8312,7 +8366,7 @@ set_config_option_ext(const char *name, const char *value, /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); break; #undef newval @@ -8351,7 +8405,7 @@ set_config_option_ext(const char *name, const char *value, { /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); if (*conf->variable != newval) { @@ -8410,7 +8464,7 @@ set_config_option_ext(const char *name, const char *value, /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); break; #undef newval @@ -8449,7 +8503,7 @@ set_config_option_ext(const char *name, const char *value, { /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); if (*conf->variable != newval) { @@ -8508,7 +8562,7 @@ set_config_option_ext(const char *name, const char *value, /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); break; #undef newval @@ -8542,7 +8596,7 @@ set_config_option_ext(const char *name, const char *value, if (!call_string_check_hook(conf, &newval, &newextra, source, elevel)) { - free(newval); + guc_free(newval); return 0; } } @@ -8570,10 +8624,10 @@ set_config_option_ext(const char *name, const char *value, /* Release newval, unless it's reset_val */ if (newval && !string_field_used(conf, newval)) - free(newval); + guc_free(newval); /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); if (newval_different) { @@ -8634,10 +8688,10 @@ set_config_option_ext(const char *name, const char *value, /* Perhaps we didn't install newval anywhere */ if (newval && !string_field_used(conf, newval)) - free(newval); + guc_free(newval); /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); break; #undef newval @@ -8676,7 +8730,7 @@ set_config_option_ext(const char *name, const char *value, { /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); if (*conf->variable != newval) { @@ -8735,7 +8789,7 @@ set_config_option_ext(const char *name, const char *value, /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(&conf->gen, newextra)) - free(newextra); + guc_free(newextra); break; #undef newval @@ -8773,7 +8827,7 @@ set_config_sourcefile(const char *name, char *sourcefile, int sourceline) return; sourcefile = guc_strdup(elevel, sourcefile); - free(record->sourcefile); + guc_free(record->sourcefile); record->sourcefile = sourcefile; record->sourceline = sourceline; } @@ -9292,8 +9346,8 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) name, value))); if (record->vartype == PGC_STRING && newval.stringval != NULL) - free(newval.stringval); - free(newextra); + guc_free(newval.stringval); + guc_free(newextra); /* * We must also reject values containing newlines, because the @@ -9801,7 +9855,7 @@ define_custom_variable(struct config_generic *variable) set_string_field(pHolder, pHolder->variable, NULL); set_string_field(pHolder, &pHolder->reset_val, NULL); - free(pHolder); + guc_free(pHolder); } /* @@ -10080,7 +10134,7 @@ MarkGUCPrefixReserved(const char *className) } /* And remember the name so we can prevent future mistakes. */ - oldcontext = MemoryContextSwitchTo(TopMemoryContext); + oldcontext = MemoryContextSwitchTo(GUCMemoryContext); reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className)); MemoryContextSwitchTo(oldcontext); } @@ -11211,9 +11265,9 @@ read_nondefault_variables(void) if (varsourcefile[0]) set_config_sourcefile(varname, varsourcefile, varsourceline); - free(varname); - free(varvalue); - free(varsourcefile); + guc_free(varname); + guc_free(varvalue); + guc_free(varsourcefile); } FreeFile(fp); @@ -11655,9 +11709,9 @@ RestoreGUCState(void *gucstate) * pointers. */ Assert(gconf->stack == NULL); - free(gconf->extra); - free(gconf->last_reported); - free(gconf->sourcefile); + guc_free(gconf->extra); + guc_free(gconf->last_reported); + guc_free(gconf->sourcefile); switch (gconf->vartype) { case PGC_BOOL: @@ -11665,7 +11719,7 @@ RestoreGUCState(void *gucstate) struct config_bool *conf = (struct config_bool *) gconf; if (conf->reset_extra && conf->reset_extra != gconf->extra) - free(conf->reset_extra); + guc_free(conf->reset_extra); break; } case PGC_INT: @@ -11673,7 +11727,7 @@ RestoreGUCState(void *gucstate) struct config_int *conf = (struct config_int *) gconf; if (conf->reset_extra && conf->reset_extra != gconf->extra) - free(conf->reset_extra); + guc_free(conf->reset_extra); break; } case PGC_REAL: @@ -11681,18 +11735,18 @@ RestoreGUCState(void *gucstate) struct config_real *conf = (struct config_real *) gconf; if (conf->reset_extra && conf->reset_extra != gconf->extra) - free(conf->reset_extra); + guc_free(conf->reset_extra); break; } case PGC_STRING: { struct config_string *conf = (struct config_string *) gconf; - free(*conf->variable); + guc_free(*conf->variable); if (conf->reset_val && conf->reset_val != *conf->variable) - free(conf->reset_val); + guc_free(conf->reset_val); if (conf->reset_extra && conf->reset_extra != gconf->extra) - free(conf->reset_extra); + guc_free(conf->reset_extra); break; } case PGC_ENUM: @@ -11700,7 +11754,7 @@ RestoreGUCState(void *gucstate) struct config_enum *conf = (struct config_enum *) gconf; if (conf->reset_extra && conf->reset_extra != gconf->extra) - free(conf->reset_extra); + guc_free(conf->reset_extra); break; } } @@ -11762,7 +11816,7 @@ RestoreGUCState(void *gucstate) /* * A little "long argument" simulation, although not quite GNU * compliant. Takes a string of the form "some-option=some value" and - * returns name = "some_option" and value = "some value" in malloc'ed + * returns name = "some_option" and value = "some value" in palloc'ed * storage. Note that '-' is converted to '_' in the option name. If * there is no '=' in the input string then value will be NULL. */ @@ -11780,15 +11834,15 @@ ParseLongOption(const char *string, char **name, char **value) if (string[equal_pos] == '=') { - *name = guc_malloc(FATAL, equal_pos + 1); + *name = palloc(equal_pos + 1); strlcpy(*name, string, equal_pos + 1); - *value = guc_strdup(FATAL, &string[equal_pos + 1]); + *value = pstrdup(&string[equal_pos + 1]); } else { /* no equal sign in string */ - *name = guc_strdup(FATAL, string); + *name = pstrdup(string); *value = NULL; } @@ -11822,8 +11876,6 @@ ProcessGUCArray(ArrayType *array, char *s; char *name; char *value; - char *namecopy; - char *valuecopy; d = array_ref(array, 1, &i, -1 /* varlenarray */ , @@ -11844,22 +11896,16 @@ ProcessGUCArray(ArrayType *array, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("could not parse setting for parameter \"%s\"", name))); - free(name); + pfree(name); continue; } - /* free malloc'd strings immediately to avoid leak upon error */ - namecopy = pstrdup(name); - free(name); - valuecopy = pstrdup(value); - free(value); - - (void) set_config_option(namecopy, valuecopy, + (void) set_config_option(name, value, context, source, action, true, 0, false); - pfree(namecopy); - pfree(valuecopy); + pfree(name); + pfree(value); pfree(s); } } @@ -12323,7 +12369,7 @@ call_string_check_hook(struct config_string *conf, char **newval, void **extra, } PG_CATCH(); { - free(*newval); + guc_free(*newval); PG_RE_THROW(); } PG_END_TRY(); @@ -12670,7 +12716,7 @@ check_timezone_abbreviations(char **newval, void **extra, GucSource source) return true; } - /* OK, load the file and produce a malloc'd TimeZoneAbbrevTable */ + /* OK, load the file and produce a guc_malloc'd TimeZoneAbbrevTable */ *extra = load_tzoffsets(*newval); /* tzparser.c returns NULL on failure, reporting via GUC_check_errmsg */ diff --git a/src/backend/utils/misc/tzparser.c b/src/backend/utils/misc/tzparser.c index 8f2c95f055..e291cb63b0 100644 --- a/src/backend/utils/misc/tzparser.c +++ b/src/backend/utils/misc/tzparser.c @@ -439,7 +439,7 @@ ParseTzFile(const char *filename, int depth, * load_tzoffsets --- read and parse the specified timezone offset file * * On success, return a filled-in TimeZoneAbbrevTable, which must have been - * malloc'd not palloc'd. On failure, return NULL, using GUC_check_errmsg + * guc_malloc'd not palloc'd. On failure, return NULL, using GUC_check_errmsg * and friends to give details of the problem. */ TimeZoneAbbrevTable * diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 45ae1b537f..7811bc0a36 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -412,6 +412,11 @@ extern ArrayType *GUCArrayAdd(ArrayType *array, const char *name, const char *va extern ArrayType *GUCArrayDelete(ArrayType *array, const char *name); extern ArrayType *GUCArrayReset(ArrayType *array); +extern void *guc_malloc(int elevel, size_t size); +extern pg_nodiscard void *guc_realloc(int elevel, void *old, size_t size); +extern char *guc_strdup(int elevel, const char *src); +extern void guc_free(void *ptr); + #ifdef EXEC_BACKEND extern void write_nondefault_variables(GucContext context); extern void read_nondefault_variables(void); diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index 190d286f1c..5dc334b61b 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -114,7 +114,7 @@ plpgsql_extra_checks_check_hook(char **newvalue, void **extra, GucSource source) list_free(elemlist); } - myextra = (int *) malloc(sizeof(int)); + myextra = (int *) guc_malloc(LOG, sizeof(int)); if (!myextra) return false; *myextra = extrachecks; commit a8d41d3fa195980ace5f2bcdd257463cc225acfd Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Mon Sep 5 16:50:19 2022 -0400 Replace sorted array of GUC variables with a hash table. Get rid of bsearch() in favor of hashed lookup. The main advantage is that it becomes far cheaper to add new GUCs, since we needn't re-sort the pointer array. Adding N new GUCs had been O(N^2 log N), but now it's closer to O(N). Also, rationalize a couple of not-very-well-thought-out APIs. GetConfigOptionByNum() is outright dangerous in a world where the set of GUCs isn't fairly static; not to mention that somebody had whacked its output around to the point where it was useless to anyone except show_all_settings(). Stop exporting that, and set things up so that the result of get_guc_variables() can be trusted even if more GUCs get added while one is still consulting it. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e123ccb629..32c9f6be8f 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5140,16 +5140,17 @@ static const char *const map_old_guc_names[] = { static MemoryContext GUCMemoryContext; /* - * Actual lookup of variables is done through this single, sorted array. + * We use a dynahash table to look up GUCs by name, or to iterate through + * all the GUCs. The gucname field is redundant with gucvar->name, but + * dynahash makes it too painful to not store the hash key separately. */ -static struct config_generic **guc_variables; - -/* Current number of variables contained in the vector */ -static int num_guc_variables; - -/* Vector capacity */ -static int size_guc_variables; +typedef struct +{ + const char *gucname; /* hash key */ + struct config_generic *gucvar; /* -> GUC's defining structure */ +} GUCHashEntry; +static HTAB *guc_hashtab; /* entries are GUCHashEntrys */ static bool guc_dirty; /* true if need to do commit/abort work */ @@ -5162,6 +5163,8 @@ static int GUCNestLevel = 0; /* 1 when in main transaction */ static struct config_generic *find_option(const char *name, bool create_placeholders, bool skip_errors, int elevel); static int guc_var_compare(const void *a, const void *b); +static uint32 guc_name_hash(const void *key, Size keysize); +static int guc_name_match(const void *key1, const void *key2, Size keysize); static void InitializeGUCOptionsFromEnvironment(void); static void InitializeOneGUCOption(struct config_generic *gconf); static void push_old_value(struct config_generic *gconf, GucAction action); @@ -5198,7 +5201,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel) ConfigVariable *item, *head, *tail; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* Parse the main config file into a list of option names and values */ ConfFileWithError = ConfigFileName; @@ -5273,9 +5277,10 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel) * need this so that we can tell below which ones have been removed from * the file since we last processed it. */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *gconf = guc_variables[i]; + struct config_generic *gconf = hentry->gucvar; gconf->status &= ~GUC_IS_IN_FILE; } @@ -5359,9 +5364,10 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel) * boot-time defaults. If such a variable can't be changed after startup, * report that and continue. */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *gconf = guc_variables[i]; + struct config_generic *gconf = hentry->gucvar; GucStack *stack; if (gconf->reset_source != PGC_S_FILE || @@ -5761,17 +5767,38 @@ discard_stack_value(struct config_generic *gconf, config_var_value *val) /* - * Fetch the sorted array pointer (exported for help_config.c's use ONLY) + * Fetch a palloc'd, sorted array of GUC struct pointers + * + * The array length is returned into *num_vars. */ struct config_generic ** -get_guc_variables(void) +get_guc_variables(int *num_vars) { - return guc_variables; + struct config_generic **result; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; + int i; + + *num_vars = hash_get_num_entries(guc_hashtab); + result = palloc(sizeof(struct config_generic *) * *num_vars); + + /* Extract pointers from the hash table */ + i = 0; + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + result[i++] = hentry->gucvar; + Assert(i == *num_vars); + + /* Sort by name */ + qsort(result, *num_vars, + sizeof(struct config_generic *), guc_var_compare); + + return result; } /* - * Build the sorted array. This is split out so that help_config.c can + * Build the GUC hash table. This is split out so that help_config.c can * extract all the variables without running all of InitializeGUCOptions. * It's not meant for use anyplace else. */ @@ -5780,7 +5807,9 @@ build_guc_variables(void) { int size_vars; int num_vars = 0; - struct config_generic **guc_vars; + HASHCTL hash_ctl; + GUCHashEntry *hentry; + bool found; int i; /* @@ -5836,74 +5865,106 @@ build_guc_variables(void) } /* - * Create table with 20% slack + * Create hash table with 20% slack */ size_vars = num_vars + num_vars / 4; - guc_vars = (struct config_generic **) - guc_malloc(FATAL, size_vars * sizeof(struct config_generic *)); - - num_vars = 0; + hash_ctl.keysize = sizeof(char *); + hash_ctl.entrysize = sizeof(GUCHashEntry); + hash_ctl.hash = guc_name_hash; + hash_ctl.match = guc_name_match; + hash_ctl.hcxt = GUCMemoryContext; + guc_hashtab = hash_create("GUC hash table", + size_vars, + &hash_ctl, + HASH_ELEM | HASH_FUNCTION | HASH_COMPARE | HASH_CONTEXT); for (i = 0; ConfigureNamesBool[i].gen.name; i++) - guc_vars[num_vars++] = &ConfigureNamesBool[i].gen; + { + struct config_generic *gucvar = &ConfigureNamesBool[i].gen; + + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &gucvar->name, + HASH_ENTER, + &found); + Assert(!found); + hentry->gucvar = gucvar; + } for (i = 0; ConfigureNamesInt[i].gen.name; i++) - guc_vars[num_vars++] = &ConfigureNamesInt[i].gen; + { + struct config_generic *gucvar = &ConfigureNamesInt[i].gen; + + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &gucvar->name, + HASH_ENTER, + &found); + Assert(!found); + hentry->gucvar = gucvar; + } for (i = 0; ConfigureNamesReal[i].gen.name; i++) - guc_vars[num_vars++] = &ConfigureNamesReal[i].gen; + { + struct config_generic *gucvar = &ConfigureNamesReal[i].gen; + + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &gucvar->name, + HASH_ENTER, + &found); + Assert(!found); + hentry->gucvar = gucvar; + } for (i = 0; ConfigureNamesString[i].gen.name; i++) - guc_vars[num_vars++] = &ConfigureNamesString[i].gen; + { + struct config_generic *gucvar = &ConfigureNamesString[i].gen; + + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &gucvar->name, + HASH_ENTER, + &found); + Assert(!found); + hentry->gucvar = gucvar; + } for (i = 0; ConfigureNamesEnum[i].gen.name; i++) - guc_vars[num_vars++] = &ConfigureNamesEnum[i].gen; + { + struct config_generic *gucvar = &ConfigureNamesEnum[i].gen; - guc_free(guc_variables); - guc_variables = guc_vars; - num_guc_variables = num_vars; - size_guc_variables = size_vars; - qsort((void *) guc_variables, num_guc_variables, - sizeof(struct config_generic *), guc_var_compare); + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &gucvar->name, + HASH_ENTER, + &found); + Assert(!found); + hentry->gucvar = gucvar; + } + + Assert(num_vars == hash_get_num_entries(guc_hashtab)); } /* - * Add a new GUC variable to the list of known variables. The - * list is expanded if needed. + * Add a new GUC variable to the hash of known variables. The + * hash is expanded if needed. */ static bool add_guc_variable(struct config_generic *var, int elevel) { - if (num_guc_variables + 1 >= size_guc_variables) - { - /* - * Increase the vector by 25% - */ - int size_vars = size_guc_variables + size_guc_variables / 4; - struct config_generic **guc_vars; - - if (size_vars == 0) - { - size_vars = 100; - guc_vars = (struct config_generic **) - guc_malloc(elevel, size_vars * sizeof(struct config_generic *)); - } - else - { - guc_vars = (struct config_generic **) - guc_realloc(elevel, guc_variables, size_vars * sizeof(struct config_generic *)); - } - - if (guc_vars == NULL) - return false; /* out of memory */ + GUCHashEntry *hentry; + bool found; - guc_variables = guc_vars; - size_guc_variables = size_vars; + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &var->name, + HASH_ENTER_NULL, + &found); + if (unlikely(hentry == NULL)) + { + ereport(elevel, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); + return false; /* out of memory */ } - guc_variables[num_guc_variables++] = var; - qsort((void *) guc_variables, num_guc_variables, - sizeof(struct config_generic *), guc_var_compare); + Assert(!found); + hentry->gucvar = var; return true; } @@ -6012,23 +6073,18 @@ static struct config_generic * find_option(const char *name, bool create_placeholders, bool skip_errors, int elevel) { - const char **key = &name; - struct config_generic **res; + GUCHashEntry *hentry; int i; Assert(name); - /* - * By equating const char ** with struct config_generic *, we are assuming - * the name field is first in config_generic. - */ - res = (struct config_generic **) bsearch((void *) &key, - (void *) guc_variables, - num_guc_variables, - sizeof(struct config_generic *), - guc_var_compare); - if (res) - return *res; + /* Look it up using the hash table. */ + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &name, + HASH_FIND, + NULL); + if (hentry) + return hentry->gucvar; /* * See if the name is an obsolete name for a variable. We assume that the @@ -6101,7 +6157,7 @@ find_option(const char *name, bool create_placeholders, bool skip_errors, /* - * comparator for qsorting and bsearching guc_variables array + * comparator for qsorting an array of GUC pointers */ static int guc_var_compare(const void *a, const void *b) @@ -6120,7 +6176,7 @@ guc_name_compare(const char *namea, const char *nameb) { /* * The temptation to use strcasecmp() here must be resisted, because the - * array ordering has to remain stable across setlocale() calls. So, build + * hash mapping has to remain stable across setlocale() calls. So, build * our own with a simple ASCII-only downcasing. */ while (*namea && *nameb) @@ -6142,6 +6198,42 @@ guc_name_compare(const char *namea, const char *nameb) return 0; } +/* + * Hash function that's compatible with guc_name_compare + */ +static uint32 +guc_name_hash(const void *key, Size keysize) +{ + uint32 result = 0; + const char *name = *(const char *const *) key; + + while (*name) + { + char ch = *name++; + + /* Case-fold in the same way as guc_name_compare */ + if (ch >= 'A' && ch <= 'Z') + ch += 'a' - 'A'; + + /* Merge into hash ... not very bright, but it needn't be */ + result = pg_rotate_left32(result, 5); + result ^= (uint32) ch; + } + return result; +} + +/* + * Dynahash match function to use in guc_hashtab + */ +static int +guc_name_match(const void *key1, const void *key2, Size keysize) +{ + const char *name1 = *(const char *const *) key1; + const char *name2 = *(const char *const *) key2; + + return guc_name_compare(name1, name2); +} + /* * Convert a GUC name to the form that should be used in pg_parameter_acl. @@ -6211,7 +6303,8 @@ check_GUC_name_for_parameter_acl(const char *name) void InitializeGUCOptions(void) { - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* * Before log_line_prefix could possibly receive a nonempty setting, make @@ -6220,7 +6313,7 @@ InitializeGUCOptions(void) pg_timezone_initialize(); /* - * Create GUCMemoryContext and build sorted array of all GUC variables. + * Create GUCMemoryContext and build hash table of all GUC variables. */ build_guc_variables(); @@ -6228,9 +6321,10 @@ InitializeGUCOptions(void) * Load all variables with their compiled-in defaults, and initialize * status fields as needed. */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - InitializeOneGUCOption(guc_variables[i]); + InitializeOneGUCOption(hentry->gucvar); } guc_dirty = false; @@ -6674,11 +6768,13 @@ SelectConfigFiles(const char *userDoption, const char *progname) void ResetAllOptions(void) { - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *gconf = guc_variables[i]; + struct config_generic *gconf = hentry->gucvar; /* Don't reset non-SET-able values */ if (gconf->context != PGC_SUSET && @@ -6896,7 +6992,8 @@ void AtEOXact_GUC(bool isCommit, int nestLevel) { bool still_dirty; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* * Note: it's possible to get here with GUCNestLevel == nestLevel-1 during @@ -6915,9 +7012,10 @@ AtEOXact_GUC(bool isCommit, int nestLevel) } still_dirty = false; - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *gconf = guc_variables[i]; + struct config_generic *gconf = hentry->gucvar; GucStack *stack; /* @@ -7186,7 +7284,8 @@ AtEOXact_GUC(bool isCommit, int nestLevel) void BeginReportingGUCOptions(void) { - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* * Don't do anything unless talking to an interactive frontend. @@ -7209,9 +7308,10 @@ BeginReportingGUCOptions(void) PGC_INTERNAL, PGC_S_OVERRIDE); /* Transmit initial values of interesting variables */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *conf = guc_variables[i]; + struct config_generic *conf = hentry->gucvar; if (conf->flags & GUC_REPORT) ReportGUCOption(conf); @@ -7236,6 +7336,9 @@ BeginReportingGUCOptions(void) void ReportChangedGUCOptions(void) { + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; + /* Quick exit if not (yet) enabled */ if (!reporting_enabled) return; @@ -7255,9 +7358,10 @@ ReportChangedGUCOptions(void) return; /* Transmit new values of interesting variables */ - for (int i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *conf = guc_variables[i]; + struct config_generic *conf = hentry->gucvar; if ((conf->flags & GUC_REPORT) && (conf->status & GUC_NEEDS_REPORT)) ReportGUCOption(conf); @@ -9761,25 +9865,23 @@ init_custom_variable(const char *name, /* * Common code for DefineCustomXXXVariable subroutines: insert the new - * variable into the GUC variable array, replacing any placeholder. + * variable into the GUC variable hash, replacing any placeholder. */ static void define_custom_variable(struct config_generic *variable) { const char *name = variable->name; - const char **nameAddr = &name; + GUCHashEntry *hentry; struct config_string *pHolder; - struct config_generic **res; /* * See if there's a placeholder by the same name. */ - res = (struct config_generic **) bsearch((void *) &nameAddr, - (void *) guc_variables, - num_guc_variables, - sizeof(struct config_generic *), - guc_var_compare); - if (res == NULL) + hentry = (GUCHashEntry *) hash_search(guc_hashtab, + &name, + HASH_FIND, + NULL); + if (hentry == NULL) { /* * No placeholder to replace, so we can just add it ... but first, @@ -9793,13 +9895,13 @@ define_custom_variable(struct config_generic *variable) /* * This better be a placeholder */ - if (((*res)->flags & GUC_CUSTOM_PLACEHOLDER) == 0) + if ((hentry->gucvar->flags & GUC_CUSTOM_PLACEHOLDER) == 0) ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("attempt to redefine parameter \"%s\"", name))); - Assert((*res)->vartype == PGC_STRING); - pHolder = (struct config_string *) (*res); + Assert(hentry->gucvar->vartype == PGC_STRING); + pHolder = (struct config_string *) hentry->gucvar; /* * First, set the variable to its default value. We must do this even @@ -9809,10 +9911,11 @@ define_custom_variable(struct config_generic *variable) InitializeOneGUCOption(variable); /* - * Replace the placeholder. We aren't changing the name, so no re-sorting - * is necessary + * Replace the placeholder in the hash table. We aren't changing the name + * (at least up to case-folding), so the hash value is unchanged. */ - *res = variable; + hentry->gucname = name; + hentry->gucvar = variable; /* * Assign the string value(s) stored in the placeholder to the real @@ -10104,7 +10207,8 @@ void MarkGUCPrefixReserved(const char *className) { int classLen = strlen(className); - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; MemoryContext oldcontext; /* @@ -10113,9 +10217,10 @@ MarkGUCPrefixReserved(const char *className) * don't bother trying to free associated memory, since this shouldn't * happen often.) */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *var = guc_variables[i]; + struct config_generic *var = hentry->gucvar; if ((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 && strncmp(className, var->name, classLen) == 0 && @@ -10127,9 +10232,10 @@ MarkGUCPrefixReserved(const char *className) var->name), errdetail("\"%s\" is now a reserved prefix.", className))); - num_guc_variables--; - memmove(&guc_variables[i], &guc_variables[i + 1], - (num_guc_variables - i) * sizeof(struct config_generic *)); + hash_search(guc_hashtab, + &var->name, + HASH_REMOVE, + NULL); } } @@ -10218,12 +10324,16 @@ ShowGUCConfigOption(const char *name, DestReceiver *dest) static void ShowAllGUCConfig(DestReceiver *dest) { - int i; + struct config_generic **guc_vars; + int num_vars; TupOutputState *tstate; TupleDesc tupdesc; Datum values[3]; bool isnull[3] = {false, false, false}; + /* collect the variables, in sorted order */ + guc_vars = get_guc_variables(&num_vars); + /* need a tuple descriptor representing three TEXT columns */ tupdesc = CreateTemplateTupleDesc(3); TupleDescInitBuiltinEntry(tupdesc, (AttrNumber) 1, "name", @@ -10236,9 +10346,9 @@ ShowAllGUCConfig(DestReceiver *dest) /* prepare for projection of tuples */ tstate = begin_tup_output_tupdesc(dest, tupdesc, &TTSOpsVirtual); - for (i = 0; i < num_guc_variables; i++) + for (int i = 0; i < num_vars; i++) { - struct config_generic *conf = guc_variables[i]; + struct config_generic *conf = guc_vars[i]; char *setting; if ((conf->flags & GUC_NO_SHOW_ALL) || @@ -10299,6 +10409,8 @@ struct config_generic ** get_explain_guc_options(int *num) { struct config_generic **result; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; *num = 0; @@ -10306,12 +10418,13 @@ get_explain_guc_options(int *num) * While only a fraction of all the GUC variables are marked GUC_EXPLAIN, * it doesn't seem worth dynamically resizing this array. */ - result = palloc(sizeof(struct config_generic *) * num_guc_variables); + result = palloc(sizeof(struct config_generic *) * hash_get_num_entries(guc_hashtab)); - for (int i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { + struct config_generic *conf = hentry->gucvar; bool modified; - struct config_generic *conf = guc_variables[i]; /* return only parameters marked for inclusion in explain */ if (!(conf->flags & GUC_EXPLAIN)) @@ -10454,19 +10567,13 @@ pg_settings_get_flags(PG_FUNCTION_ARGS) } /* - * Return GUC variable value by variable number; optionally return canonical - * form of name. Return value is palloc'd. + * Extract fields to show in pg_settings for given variable. */ -void -GetConfigOptionByNum(int varnum, const char **values, bool *noshow) +static void +GetConfigOptionValues(struct config_generic *conf, const char **values, + bool *noshow) { char buffer[256]; - struct config_generic *conf; - - /* check requested variable number valid */ - Assert((varnum >= 0) && (varnum < num_guc_variables)); - - conf = guc_variables[varnum]; if (noshow) { @@ -10682,15 +10789,6 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) values[16] = (conf->status & GUC_PENDING_RESTART) ? "t" : "f"; } -/* - * Return the total number of GUC variables - */ -int -GetNumConfigOptions(void) -{ - return num_guc_variables; -} - /* * show_config_by_name - equiv to SHOW X command but implemented as * a function. @@ -10741,6 +10839,8 @@ Datum show_all_settings(PG_FUNCTION_ARGS) { FuncCallContext *funcctx; + struct config_generic **guc_vars; + int num_vars; TupleDesc tupdesc; int call_cntr; int max_calls; @@ -10805,8 +10905,14 @@ show_all_settings(PG_FUNCTION_ARGS) attinmeta = TupleDescGetAttInMetadata(tupdesc); funcctx->attinmeta = attinmeta; + /* collect the variables, in sorted order */ + guc_vars = get_guc_variables(&num_vars); + + /* use user_fctx to remember the array location */ + funcctx->user_fctx = guc_vars; + /* total number of tuples to be returned */ - funcctx->max_calls = GetNumConfigOptions(); + funcctx->max_calls = num_vars; MemoryContextSwitchTo(oldcontext); } @@ -10814,6 +10920,7 @@ show_all_settings(PG_FUNCTION_ARGS) /* stuff done on every call of the function */ funcctx = SRF_PERCALL_SETUP(); + guc_vars = (struct config_generic **) funcctx->user_fctx; call_cntr = funcctx->call_cntr; max_calls = funcctx->max_calls; attinmeta = funcctx->attinmeta; @@ -10830,7 +10937,8 @@ show_all_settings(PG_FUNCTION_ARGS) */ do { - GetConfigOptionByNum(call_cntr, (const char **) values, &noshow); + GetConfigOptionValues(guc_vars[call_cntr], (const char **) values, + &noshow); if (noshow) { /* bump the counter and get the next config setting */ @@ -11131,7 +11239,8 @@ write_nondefault_variables(GucContext context) { int elevel; FILE *fp; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP); @@ -11150,9 +11259,10 @@ write_nondefault_variables(GucContext context) return; } - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - write_one_nondefault_variable(fp, guc_variables[i]); + write_one_nondefault_variable(fp, hentry->gucvar); } if (FreeFile(fp)) @@ -11428,15 +11538,17 @@ Size EstimateGUCStateSpace(void) { Size size; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* Add space reqd for saving the data size of the guc state */ size = sizeof(Size); /* Add up the space needed for each GUC variable */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) size = add_size(size, - estimate_variable_size(guc_variables[i])); + estimate_variable_size(hentry->gucvar)); return size; } @@ -11575,15 +11687,17 @@ SerializeGUCState(Size maxsize, char *start_address) char *curptr; Size actual_size; Size bytes_left; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; /* Reserve space for saving the actual size of the guc state */ Assert(maxsize > sizeof(actual_size)); curptr = start_address + sizeof(actual_size); bytes_left = maxsize - sizeof(actual_size); - for (i = 0; i < num_guc_variables; i++) - serialize_variable(&curptr, &bytes_left, guc_variables[i]); + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + serialize_variable(&curptr, &bytes_left, hentry->gucvar); /* Store actual size without assuming alignment of start_address. */ actual_size = maxsize - bytes_left - sizeof(actual_size); @@ -11668,7 +11782,8 @@ RestoreGUCState(void *gucstate) char *srcptr = (char *) gucstate; char *srcend; Size len; - int i; + HASH_SEQ_STATUS status; + GUCHashEntry *hentry; ErrorContextCallback error_context_callback; /* @@ -11693,9 +11808,10 @@ RestoreGUCState(void *gucstate) * also ensures that set_config_option won't refuse to set them because of * source-priority comparisons. */ - for (i = 0; i < num_guc_variables; i++) + hash_seq_init(&status, guc_hashtab); + while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) { - struct config_generic *gconf = guc_variables[i]; + struct config_generic *gconf = hentry->gucvar; /* Do nothing if non-shippable or if already at PGC_S_DEFAULT. */ if (can_skip_gucvar(gconf)) diff --git a/src/backend/utils/misc/help_config.c b/src/backend/utils/misc/help_config.c index 61c83f3590..59d2e36548 100644 --- a/src/backend/utils/misc/help_config.c +++ b/src/backend/utils/misc/help_config.c @@ -49,11 +49,10 @@ GucInfoMain(void) int numOpts, i; - /* Initialize the guc_variables[] array */ + /* Initialize the GUC hash table */ build_guc_variables(); - guc_vars = get_guc_variables(); - numOpts = GetNumConfigOptions(); + guc_vars = get_guc_variables(&numOpts); for (i = 0; i < numOpts; i++) { diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index 7811bc0a36..ffb00999d3 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -396,8 +396,6 @@ extern int set_config_option_ext(const char *name, const char *value, extern void AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt); extern char *GetConfigOptionByName(const char *name, const char **varname, bool missing_ok); -extern void GetConfigOptionByNum(int varnum, const char **values, bool *noshow); -extern int GetNumConfigOptions(void); extern void SetPGVariable(const char *name, List *args, bool is_local); extern void GetPGVariable(const char *name, DestReceiver *dest); diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h index 90565b9921..97e86f01cb 100644 --- a/src/include/utils/guc_tables.h +++ b/src/include/utils/guc_tables.h @@ -264,7 +264,7 @@ extern PGDLLIMPORT const char *const GucContext_Names[]; extern PGDLLIMPORT const char *const GucSource_Names[]; /* get the current set of variables */ -extern struct config_generic **get_guc_variables(void); +extern struct config_generic **get_guc_variables(int *num_vars); extern void build_guc_variables(void); commit 88f8274143e95686c64dd2f23ed22bf6b12759a9 Author: Tom Lane <tgl@sss.pgh.pa.us> Date: Mon Sep 5 17:10:07 2022 -0400 Add auxiliary lists to GUC data structures for performance. The previous patch made addition of new GUCs cheap, but other GUC operations aren't improved and indeed get a bit slower, because hash_seq_search() is slower than just scanning a pointer array. However, most performance-critical GUC operations only need to touch a relatively small fraction of the GUCs; especially so for AtEOXact_GUC(). We can improve matters at the cost of a bit more space by adding dlist or slist links to the GUC data structures. This patch invents lists that track (1) all GUCs with non-default "source"; (2) all GUCs with nonempty state stack (implying they've been changed in the current transaction); (3) all GUCs due for reporting to the client. All of the performance-critical cases can make use of one or another of these lists to avoid searching the hash table. In particular, the stack list means that transaction end doesn't take time proportional to the number of GUCs, but only the number changed in the current transaction. diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 32c9f6be8f..bbfaca92ea 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5152,12 +5152,23 @@ typedef struct static HTAB *guc_hashtab; /* entries are GUCHashEntrys */ -static bool guc_dirty; /* true if need to do commit/abort work */ +/* + * In addition to the hash table, variables having certain properties are + * linked into these lists, so that we can find them without scanning the + * whole hash table. In most applications, only a small fraction of the + * GUCs appear in these lists at any given time. The usage of the stack + * and report lists is stylized enough that they can be slists, but the + * nondef list has to be a dlist to avoid O(N) deletes in common cases. + */ +static dlist_head guc_nondef_list; /* list of variables that have source + * different from PGC_S_DEFAULT */ +static slist_head guc_stack_list; /* list of variables that have non-NULL + * stack */ +static slist_head guc_report_list; /* list of variables that have the + * GUC_NEEDS_REPORT bit set in status */ static bool reporting_enabled; /* true to enable GUC_REPORT */ -static bool report_needed; /* true if any GUC_REPORT reports are needed */ - static int GUCNestLevel = 0; /* 1 when in main transaction */ @@ -5167,6 +5178,8 @@ static uint32 guc_name_hash(const void *key, Size keysize); static int guc_name_match(const void *key1, const void *key2, Size keysize); static void InitializeGUCOptionsFromEnvironment(void); static void InitializeOneGUCOption(struct config_generic *gconf); +static void RemoveGUCFromLists(struct config_generic *gconf); +static void set_guc_source(struct config_generic *gconf, GucSource newsource); static void push_old_value(struct config_generic *gconf, GucAction action); static void ReportGUCOption(struct config_generic *record); static void reapply_stacked_values(struct config_generic *variable, @@ -5400,7 +5413,7 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel) if (gconf->reset_source == PGC_S_FILE) gconf->reset_source = PGC_S_DEFAULT; if (gconf->source == PGC_S_FILE) - gconf->source = PGC_S_DEFAULT; + set_guc_source(gconf, PGC_S_DEFAULT); for (stack = gconf->stack; stack; stack = stack->prev) { if (stack->source == PGC_S_FILE) @@ -6327,8 +6340,6 @@ InitializeGUCOptions(void) InitializeOneGUCOption(hentry->gucvar); } - guc_dirty = false; - reporting_enabled = false; /* @@ -6553,6 +6564,23 @@ InitializeOneGUCOption(struct config_generic *gconf) } } +/* + * Summarily remove a GUC variable from any linked lists it's in. + * + * We use this in cases where the variable is about to be deleted or reset. + * These aren't common operations, so it's okay if this is a bit slow. + */ +static void +RemoveGUCFromLists(struct config_generic *gconf) +{ + if (gconf->source != PGC_S_DEFAULT) + dlist_delete(&gconf->nondef_link); + if (gconf->stack != NULL) + slist_delete(&guc_stack_list, &gconf->stack_link); + if (gconf->status & GUC_NEEDS_REPORT) + slist_delete(&guc_report_list, &gconf->report_link); +} + /* * Select the configuration files and data directory to be used, and @@ -6768,13 +6796,13 @@ SelectConfigFiles(const char *userDoption, const char *progname) void ResetAllOptions(void) { - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_mutable_iter iter; - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + /* We need only consider GUCs not already at PGC_S_DEFAULT */ + dlist_foreach_modify(iter, &guc_nondef_list) { - struct config_generic *gconf = hentry->gucvar; + struct config_generic *gconf = dlist_container(struct config_generic, + nondef_link, iter.cur); /* Don't reset non-SET-able values */ if (gconf->context != PGC_SUSET && @@ -6854,19 +6882,44 @@ ResetAllOptions(void) } } - gconf->source = gconf->reset_source; + set_guc_source(gconf, gconf->reset_source); gconf->scontext = gconf->reset_scontext; gconf->srole = gconf->reset_srole; - if (gconf->flags & GUC_REPORT) + if ((gconf->flags & GUC_REPORT) && !(gconf->status & GUC_NEEDS_REPORT)) { gconf->status |= GUC_NEEDS_REPORT; - report_needed = true; + slist_push_head(&guc_report_list, &gconf->report_link); } } } +/* + * Apply a change to a GUC variable's "source" field. + * + * Use this rather than just assigning, to ensure that the variable's + * membership in guc_nondef_list is updated correctly. + */ +static void +set_guc_source(struct config_generic *gconf, GucSource newsource) +{ + /* Adjust nondef list membership if appropriate for change */ + if (gconf->source == PGC_S_DEFAULT) + { + if (newsource != PGC_S_DEFAULT) + dlist_push_tail(&guc_nondef_list, &gconf->nondef_link); + } + else + { + if (newsource == PGC_S_DEFAULT) + dlist_delete(&gconf->nondef_link); + } + /* Now update the source field */ + gconf->source = newsource; +} + + /* * push_old_value * Push previous state during transactional assignment to a GUC variable. @@ -6913,7 +6966,6 @@ push_old_value(struct config_generic *gconf, GucAction action) Assert(stack->state == GUC_SAVE); break; } - Assert(guc_dirty); /* must be set already */ return; } @@ -6944,10 +6996,9 @@ push_old_value(struct config_generic *gconf, GucAction action) stack->srole = gconf->srole; set_stack_value(gconf, &stack->prior); + if (gconf->stack == NULL) + slist_push_head(&guc_stack_list, &gconf->stack_link); gconf->stack = stack; - - /* Ensure we remember to pop at end of xact */ - guc_dirty = true; } @@ -6991,9 +7042,7 @@ NewGUCNestLevel(void) void AtEOXact_GUC(bool isCommit, int nestLevel) { - bool still_dirty; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + slist_mutable_iter iter; /* * Note: it's possible to get here with GUCNestLevel == nestLevel-1 during @@ -7004,18 +7053,11 @@ AtEOXact_GUC(bool isCommit, int nestLevel) (nestLevel <= GUCNestLevel || (nestLevel == GUCNestLevel + 1 && !isCommit))); - /* Quick exit if nothing's changed in this transaction */ - if (!guc_dirty) + /* We need only process GUCs having nonempty stacks */ + slist_foreach_modify(iter, &guc_stack_list) { - GUCNestLevel = nestLevel - 1; - return; - } - - still_dirty = false; - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) - { - struct config_generic *gconf = hentry->gucvar; + struct config_generic *gconf = slist_container(struct config_generic, + stack_link, iter.cur); GucStack *stack; /* @@ -7248,30 +7290,30 @@ AtEOXact_GUC(bool isCommit, int nestLevel) set_extra_field(gconf, &(stack->masked.extra), NULL); /* And restore source information */ - gconf->source = newsource; + set_guc_source(gconf, newsource); gconf->scontext = newscontext; gconf->srole = newsrole; } - /* Finish popping the state stack */ + /* + * Pop the GUC's state stack; if it's now empty, remove the GUC + * from guc_stack_list. + */ gconf->stack = prev; + if (prev == NULL) + slist_delete_current(&iter); pfree(stack); /* Report new value if we changed it */ - if (changed && (gconf->flags & GUC_REPORT)) + if (changed && (gconf->flags & GUC_REPORT) && + !(gconf->status & GUC_NEEDS_REPORT)) { gconf->status |= GUC_NEEDS_REPORT; - report_needed = true; + slist_push_head(&guc_report_list, &gconf->report_link); } } /* end of stack-popping loop */ - - if (stack != NULL) - still_dirty = true; } - /* If there are no remaining stack entries, we can reset guc_dirty */ - guc_dirty = still_dirty; - /* Update nesting level */ GUCNestLevel = nestLevel - 1; } @@ -7316,8 +7358,6 @@ BeginReportingGUCOptions(void) if (conf->flags & GUC_REPORT) ReportGUCOption(conf); } - - report_needed = false; } /* @@ -7336,8 +7376,7 @@ BeginReportingGUCOptions(void) void ReportChangedGUCOptions(void) { - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + slist_mutable_iter iter; /* Quick exit if not (yet) enabled */ if (!reporting_enabled) @@ -7353,28 +7392,24 @@ ReportChangedGUCOptions(void) SetConfigOption("in_hot_standby", "false", PGC_INTERNAL, PGC_S_OVERRIDE); - /* Quick exit if no values have been changed */ - if (!report_needed) - return; - /* Transmit new values of interesting variables */ - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + slist_foreach_modify(iter, &guc_report_list) { - struct config_generic *conf = hentry->gucvar; + struct config_generic *conf = slist_container(struct config_generic, + report_link, iter.cur); - if ((conf->flags & GUC_REPORT) && (conf->status & GUC_NEEDS_REPORT)) - ReportGUCOption(conf); + Assert((conf->flags & GUC_REPORT) && (conf->status & GUC_NEEDS_REPORT)); + ReportGUCOption(conf); + conf->status &= ~GUC_NEEDS_REPORT; + slist_delete_current(&iter); } - - report_needed = false; } /* * ReportGUCOption: if appropriate, transmit option value to frontend * * We need not transmit the value if it's the same as what we last - * transmitted. However, clear the NEEDS_REPORT flag in any case. + * transmitted. */ static void ReportGUCOption(struct config_generic *record) @@ -7401,8 +7436,6 @@ ReportGUCOption(struct config_generic *record) } pfree(val); - - record->status &= ~GUC_NEEDS_REPORT; } /* @@ -8437,7 +8470,7 @@ set_config_option_ext(const char *name, const char *value, *conf->variable = newval; set_extra_field(&conf->gen, &conf->gen.extra, newextra); - conf->gen.source = source; + set_guc_source(&conf->gen, source); conf->gen.scontext = context; conf->gen.srole = srole; } @@ -8535,7 +8568,7 @@ set_config_option_ext(const char *name, const char *value, *conf->variable = newval; set_extra_field(&conf->gen, &conf->gen.extra, newextra); - conf->gen.source = source; + set_guc_source(&conf->gen, source); conf->gen.scontext = context; conf->gen.srole = srole; } @@ -8633,7 +8666,7 @@ set_config_option_ext(const char *name, const char *value, *conf->variable = newval; set_extra_field(&conf->gen, &conf->gen.extra, newextra); - conf->gen.source = source; + set_guc_source(&conf->gen, source); conf->gen.scontext = context; conf->gen.srole = srole; } @@ -8757,7 +8790,7 @@ set_config_option_ext(const char *name, const char *value, set_string_field(conf, conf->variable, newval); set_extra_field(&conf->gen, &conf->gen.extra, newextra); - conf->gen.source = source; + set_guc_source(&conf->gen, source); conf->gen.scontext = context; conf->gen.srole = srole; } @@ -8860,7 +8893,7 @@ set_config_option_ext(const char *name, const char *value, *conf->variable = newval; set_extra_field(&conf->gen, &conf->gen.extra, newextra); - conf->gen.source = source; + set_guc_source(&conf->gen, source); conf->gen.scontext = context; conf->gen.srole = srole; } @@ -8900,10 +8933,11 @@ set_config_option_ext(const char *name, const char *value, } } - if (changeVal && (record->flags & GUC_REPORT)) + if (changeVal && (record->flags & GUC_REPORT) && + !(record->status & GUC_NEEDS_REPORT)) { record->status |= GUC_NEEDS_REPORT; - report_needed = true; + slist_push_head(&guc_report_list, &record->report_link); } return changeVal ? 1 : -1; @@ -9917,6 +9951,11 @@ define_custom_variable(struct config_generic *variable) hentry->gucname = name; hentry->gucvar = variable; + /* + * Remove the placeholder from any lists it's in, too. + */ + RemoveGUCFromLists(&pHolder->gen); + /* * Assign the string value(s) stored in the placeholder to the real * variable. Essentially, we need to duplicate all the active and stacked @@ -10048,7 +10087,11 @@ reapply_stacked_values(struct config_generic *variable, (void) set_config_option_ext(name, curvalue, curscontext, cursource, cursrole, GUC_ACTION_SET, true, WARNING, false); - variable->stack = NULL; + if (variable->stack != NULL) + { + slist_delete(&guc_stack_list, &variable->stack_link); + variable->stack = NULL; + } } } } @@ -10232,10 +10275,13 @@ MarkGUCPrefixReserved(const char *className) var->name), errdetail("\"%s\" is now a reserved prefix.", className))); + /* Remove it from the hash table */ hash_search(guc_hashtab, &var->name, HASH_REMOVE, NULL); + /* Remove it from any lists it's in, too */ + RemoveGUCFromLists(var); } } @@ -10409,8 +10455,7 @@ struct config_generic ** get_explain_guc_options(int *num) { struct config_generic **result; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_iter iter; *num = 0; @@ -10420,10 +10465,11 @@ get_explain_guc_options(int *num) */ result = palloc(sizeof(struct config_generic *) * hash_get_num_entries(guc_hashtab)); - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + /* We need only consider GUCs with source not PGC_S_DEFAULT */ + dlist_foreach(iter, &guc_nondef_list) { - struct config_generic *conf = hentry->gucvar; + struct config_generic *conf = dlist_container(struct config_generic, + nondef_link, iter.cur); bool modified; /* return only parameters marked for inclusion in explain */ @@ -11169,8 +11215,7 @@ _ShowOption(struct config_generic *record, bool use_units) static void write_one_nondefault_variable(FILE *fp, struct config_generic *gconf) { - if (gconf->source == PGC_S_DEFAULT) - return; + Assert(gconf->source != PGC_S_DEFAULT); fprintf(fp, "%s", gconf->name); fputc(0, fp); @@ -11239,8 +11284,7 @@ write_nondefault_variables(GucContext context) { int elevel; FILE *fp; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_iter iter; Assert(context == PGC_POSTMASTER || context == PGC_SIGHUP); @@ -11259,10 +11303,13 @@ write_nondefault_variables(GucContext context) return; } - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + /* We need only consider GUCs with source not PGC_S_DEFAULT */ + dlist_foreach(iter, &guc_nondef_list) { - write_one_nondefault_variable(fp, hentry->gucvar); + struct config_generic *gconf = dlist_container(struct config_generic, + nondef_link, iter.cur); + + write_one_nondefault_variable(fp, gconf); } if (FreeFile(fp)) @@ -11538,17 +11585,23 @@ Size EstimateGUCStateSpace(void) { Size size; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_iter iter; /* Add space reqd for saving the data size of the guc state */ size = sizeof(Size); - /* Add up the space needed for each GUC variable */ - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) - size = add_size(size, - estimate_variable_size(hentry->gucvar)); + /* + * Add up the space needed for each GUC variable. + * + * We need only process non-default GUCs. + */ + dlist_foreach(iter, &guc_nondef_list) + { + struct config_generic *gconf = dlist_container(struct config_generic, + nondef_link, iter.cur); + + size = add_size(size, estimate_variable_size(gconf)); + } return size; } @@ -11687,17 +11740,21 @@ SerializeGUCState(Size maxsize, char *start_address) char *curptr; Size actual_size; Size bytes_left; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_iter iter; /* Reserve space for saving the actual size of the guc state */ Assert(maxsize > sizeof(actual_size)); curptr = start_address + sizeof(actual_size); bytes_left = maxsize - sizeof(actual_size); - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) - serialize_variable(&curptr, &bytes_left, hentry->gucvar); + /* We need only consider GUCs with source not PGC_S_DEFAULT */ + dlist_foreach(iter, &guc_nondef_list) + { + struct config_generic *gconf = dlist_container(struct config_generic, + nondef_link, iter.cur); + + serialize_variable(&curptr, &bytes_left, gconf); + } /* Store actual size without assuming alignment of start_address. */ actual_size = maxsize - bytes_left - sizeof(actual_size); @@ -11782,8 +11839,7 @@ RestoreGUCState(void *gucstate) char *srcptr = (char *) gucstate; char *srcend; Size len; - HASH_SEQ_STATUS status; - GUCHashEntry *hentry; + dlist_mutable_iter iter; ErrorContextCallback error_context_callback; /* @@ -11808,10 +11864,10 @@ RestoreGUCState(void *gucstate) * also ensures that set_config_option won't refuse to set them because of * source-priority comparisons. */ - hash_seq_init(&status, guc_hashtab); - while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL) + dlist_foreach_modify(iter, &guc_nondef_list) { - struct config_generic *gconf = hentry->gucvar; + struct config_generic *gconf = dlist_container(struct config_generic, + nondef_link, iter.cur); /* Do nothing if non-shippable or if already at PGC_S_DEFAULT. */ if (can_skip_gucvar(gconf)) @@ -11822,7 +11878,8 @@ RestoreGUCState(void *gucstate) * first we must free any existing subsidiary data to avoid leaking * memory. The stack must be empty, but we have to clean up all other * fields. Beware that there might be duplicate value or "extra" - * pointers. + * pointers. We also have to be sure to take it out of any lists it's + * in. */ Assert(gconf->stack == NULL); guc_free(gconf->extra); @@ -11874,6 +11931,8 @@ RestoreGUCState(void *gucstate) break; } } + /* Remove it from any lists it's in. */ + RemoveGUCFromLists(gconf); /* Now we can reset the struct to PGS_S_DEFAULT state. */ InitializeOneGUCOption(gconf); } diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h index 97e86f01cb..fe0fffff10 100644 --- a/src/include/utils/guc_tables.h +++ b/src/include/utils/guc_tables.h @@ -14,6 +14,7 @@ #ifndef GUC_TABLES_H #define GUC_TABLES_H 1 +#include "lib/ilist.h" #include "utils/guc.h" /* @@ -138,6 +139,11 @@ typedef struct guc_stack * if the value came from an internal source or the config file. Similarly * for reset_srole (which is usually BOOTSTRAP_SUPERUSERID, but not always). * + * Variables that are currently of active interest for maintenance + * operations are linked into various lists using the xxx_link fields. + * The link fields are unused/garbage in variables not currently having + * the specified properties. + * * Note that sourcefile/sourceline are kept here, and not pushed into stacked * values, although in principle they belong with some stacked value if the * active value is session- or transaction-local. This is to avoid bloating @@ -163,6 +169,12 @@ struct config_generic Oid reset_srole; /* role that set the reset value */ GucStack *stack; /* stacked prior values */ void *extra; /* "extra" pointer for current actual value */ + dlist_node nondef_link; /* list link for variables that have source + * different from PGC_S_DEFAULT */ + slist_node stack_link; /* list link for variables that have non-NULL + * stack */ + slist_node report_link; /* list link for variables that have the + * GUC_NEEDS_REPORT bit set in status */ char *last_reported; /* if variable is GUC_REPORT, value last sent * to client (NULL if not yet sent) */ char *sourcefile; /* file current setting is from (NULL if not
pgsql-hackers by date: