Thread

  1. GUC thread-safety approaches

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-18T08:50:37Z

    I want to discuss possible approaches to making the GUC system
    thread-safe.  In particular, I want to talk about the global
    variables.
    
    A GUC parameter is defined via a struct config_generic record that
    contains some metadata and a pointer to a global variable.  For
    example (simplified):
    
         // elsewhere
         bool enable_seqscan = true;
    
         // elsewhere
         extern bool enable_seqscan;
    
         // in guc_tables.inc.c (generated)
         ...
         {
             .name = "enable_seqscan",
             .context = PGC_USERSET,
             .group = QUERY_TUNING_METHOD,
             .short_desc = gettext_noop("Enables the planner's use of 
    sequential-scan plans."),
             .flags = GUC_EXPLAIN,
             .vartype = PGC_BOOL,
             ._bool = {
                 .variable = &enable_seqscan,  // HERE
                 .boot_val = true,
             },
         },
    
    For a multithreaded server, one of the ideas thrown around was to
    convert (most) global variables to thread-local variables.  That way,
    the overall structure of the code could remain the same, and each
    thread would see the same set of global variables as before.
    
    So you could do
    
         thread_local bool enable_seqscan = true;
    
    and
    
         extern thread_local bool enable_seqscan;
    
    and as far as the code in optimizer/path/costsize.c or wherever is
    concerned, it would work the same ways as before.
    
    But that doesn't work because:
    
    src/include/utils/guc_tables.inc.c:1617:37: error: initializer element 
    is not constant
      1617 |                         .variable = &enable_seqscan,
    
    Heikki had developed a workaround for this in his branch[0]: For each 
    GUC parameter, create a simple function that returns the address of the
    variable, and the config_generic record stores the address of the
    function.  So like this:
    
         static bool *enable_seqscan_address(void) { return &enable_seqscan; }
    
         {
             .name = "enable_seqscan",
             .context = PGC_USERSET,
             .group = QUERY_TUNING_METHOD,
             .short_desc = gettext_noop("Enables the planner's use of 
    sequential-scan plans."),
             .flags = GUC_EXPLAIN,
             .vartype = PGC_BOOL,
             ._bool = {
                 .var_addr_func = enable_seqscan_address,  // HERE
                 .boot_val = true,
             },
         },
    
    and then the code in guc.c that reads and sets the values is adjusted
    like this in several places:
    
         -                   *conf->variable = conf->reset_val;
         +                   *conf->var_addr_func() = conf->reset_val;
    
    This works.
    
    [0]: see https://wiki.postgresql.org/wiki/Multithreading
    
    Heikki's branch contains some macros to generate those helper
    functions:
    
         #define DEFINE_BOOL_GUC_ADDR(guc) \
            static bool *guc##_address(void) { return &guc; }
    
         DEFINE_BOOL_GUC_ADDR(enable_seqscan)
    
    Note that this requires fixing up every GUC variable definition like
    this.
    
    With the generated guc_tables.inc.c, we could now generate these
    helper functions automatically.  But you'd still need to modify each
    variable definition to add the thread_local specification.
    
    Actually, in Heikki's branch this is hidden behind macros and looks
    like this:
    
         session_guc bool enable_seqscan = true;
    
    And then there is additional tooling to check the annotations of all
    global variables, GUC or not, like this.
    
    So with that approach, we could add these kinds of annotations first,
    independent of thread support, and then later on add thread support
    without any further global code changes.
    
    This, however, doesn't work for user-defined GUC parameters in
    extensions.
    
    The interface for that looks like this:
    
         DefineCustomBoolVariable("auto_explain.log_analyze",
                                  "Use EXPLAIN ANALYZE for plan logging.",
                                  NULL,
                                  &auto_explain_log_analyze,  // pointer to 
    global var
                                  false,
                                  PGC_SUSET,
                                  0,
                                  NULL,
                                  NULL,
                                  NULL);
    
    In Heikki's branch, the signature of this and related functions are
    changed like this:
    
          extern void DefineCustomBoolVariable(const char *name,
                                              const char *short_desc,
                                              const char *long_desc,
         -                                    bool *valueAddr,
         +                                    GucBoolAddressHook addr_hook,
                                              bool bootValue,
                                              GucContext context,
                                              int flags,
    
    And then there are macros like shown earlier and some other ones to
    define the required helper functions and hook this all together.
    
    As written, this would break source-code compatibility for all
    extensions that use these functions.  We could conceivably create
    alternative functions like DefineCustomBoolVariableExt() and make the
    old interfaces wrappers around the new ones, or something like that.
    But of course, we would ideally want extensions to adopt the new
    system, whatever it might be, before long.
    
    The point is, while we could probably do this transition with
    relatively little impact on the core code and built-in GUC parameters,
    it appears that extension code will require nontrivial manual work to
    adopt this and also maintain backward compatibility.  So we need to
    think this through before shipping those interfaces.
    
    Now consider furthermore that in some future we might want to decouple
    sessions from threads.  There is a lot of work to be done between here
    and there, but it seems a quite plausible idea.  At that point, we
    would need to get rid of the thread-local global variables anyway.  So
    should we do that now already?  If we're going to force extension
    authors to amend their code for this, can we do it so that they only
    have to do it once?  It would be kind of annoying if one had to
    support like three different custom-GUC interfaces in an extension
    that wants to support five PostgreSQL major versions.
    
    What might take the place of the global variables then?  Note that it
    cannot just be a struct with fields for all the parameters, because
    that's not extensible.  So it would need to be some kind of dynamic
    key-value structure, like a hash table.  And we already have that!
    All the GUC records are already in a hash table
    
         static HTAB *guc_hashtab;
    
    which is used for all the utility commands and system views and so on.
    
    Could we use that for getting the current values at run time, too?
    
    So instead of
    
         void
         cost_seqscan(...)
         {
             ...
             path->disabled_nodes = enable_seqscan ? 0 : 1;
             ...
         }
    
    do something like
    
         void
         cost_seqscan(...)
         {
             ...
             path->disabled_nodes = get_config_val_bool("enable_seqscan") ? 
    0 : 1;
             ...
         }
    
    where get_config_val_*() would be a thin wrapper around hash_search()
    (a bit like the existing GetConfigOption() and find_option(), but
    without all the error checking).
    
    Would that be too expensive?  This would have to be checked in detail,
    of course, but just for this example I note that cost_seqscan() is not
    afraid to do multiple hash table lookups anyway (e.g.,
    get_tablespace_page_costs(), get_restriction_qual_cost()), so this
    would not be an order-of-magnitude change.  There might also be other
    approaches, like caching some planner settings in PlannerInfo.  Worst
    case, as a transition measure, we could add assign hooks that write to
    a global variable on a case-by-case basis.
    
    My question at this point is, which of these scenarios should we work
    toward?  Either work toward thread-local variables and helper
    functions and provide new APIs for extensions.  Or work toward getting
    rid of the global variables and use hash-table lookups whenever the
    value is needed, with some caching if necessary.  (Or other ideas?)
    
    
    
    
  2. Re: GUC thread-safety approaches

    Heikki Linnakangas <hlinnaka@iki.fi> — 2025-11-18T14:15:36Z

    Thanks for looking into this!
    
    On 18/11/2025 10:50, Peter Eisentraut wrote:
    > In Heikki's branch, the signature of this and related functions are
    > changed like this:
    > 
    >       extern void DefineCustomBoolVariable(const char *name,
    >                                           const char *short_desc,
    >                                           const char *long_desc,
    >      -                                    bool *valueAddr,
    >      +                                    GucBoolAddressHook addr_hook,
    >                                           bool bootValue,
    >                                           GucContext context,
    >                                           int flags,
    > 
    > And then there are macros like shown earlier and some other ones to
    > define the required helper functions and hook this all together.
    > 
    > As written, this would break source-code compatibility for all
    > extensions that use these functions.  We could conceivably create
    > alternative functions like DefineCustomBoolVariableExt() and make the
    > old interfaces wrappers around the new ones, or something like that.
    > But of course, we would ideally want extensions to adopt the new
    > system, whatever it might be, before long.
    > 
    > The point is, while we could probably do this transition with
    > relatively little impact on the core code and built-in GUC parameters,
    > it appears that extension code will require nontrivial manual work to
    > adopt this and also maintain backward compatibility.  So we need to
    > think this through before shipping those interfaces.
    
    I believe it's unavoidable that extensions need to be changed. If there 
    was a backwards-compatible way to do this, we could use it in the core 
    too. So it comes down to how straightforward and mechanic we can make 
    the migration to the new paradigm. If we can write a simple example 
    snippet on how to do the migration, that's not too bad. We make small 
    mechanical changes like that, requiring extensions to be updated, in 
    every release. IIRC we added an argument to the DefineCustomXXXVariable 
    functions not that long ago (edit: ok, I checked, it was in 9.1).
    
    One important consideration is whether it's possible to write a 
    compatibility macro that makes the *new* method work when compiling 
    against *older* PostgreSQL versions. That greatly reduces the pain for 
    extensions, which usually need to be source-code compatible with 
    multiple PostgreSQL versions, as then you can just switch to the new 
    convention and rely on the compatibility macros to make it work on older 
    versions, instead sprinkling the code with #ifdef PG_VERSION_NUM blocks.
    
    > Now consider furthermore that in some future we might want to decouple
    > sessions from threads.  There is a lot of work to be done between here
    > and there, but it seems a quite plausible idea.  At that point, we
    > would need to get rid of the thread-local global variables anyway.  So
    > should we do that now already?  If we're going to force extension
    > authors to amend their code for this, can we do it so that they only
    > have to do it once?  It would be kind of annoying if one had to
    > support like three different custom-GUC interfaces in an extension
    > that wants to support five PostgreSQL major versions.
    > 
    > What might take the place of the global variables then?  Note that it
    > cannot just be a struct with fields for all the parameters, because
    > that's not extensible.  So it would need to be some kind of dynamic
    > key-value structure, like a hash table.  And we already have that!
    > All the GUC records are already in a hash table
    > 
    >      static HTAB *guc_hashtab;
    > 
    > which is used for all the utility commands and system views and so on.
    > 
    > Could we use that for getting the current values at run time, too?
    > 
    > So instead of
    > 
    >      void
    >      cost_seqscan(...)
    >      {
    >          ...
    >          path->disabled_nodes = enable_seqscan ? 0 : 1;
    >          ...
    >      }
    > 
    > do something like
    > 
    >      void
    >      cost_seqscan(...)
    >      {
    >          ...
    >          path->disabled_nodes = get_config_val_bool("enable_seqscan") ? 
    > 0 : 1;
    >          ...
    >      }
    > 
    > where get_config_val_*() would be a thin wrapper around hash_search()
    > (a bit like the existing GetConfigOption() and find_option(), but
    > without all the error checking).
    > 
    > Would that be too expensive?  This would have to be checked in detail,
    > of course, but just for this example I note that cost_seqscan() is not
    > afraid to do multiple hash table lookups anyway (e.g.,
    > get_tablespace_page_costs(), get_restriction_qual_cost()), so this
    > would not be an order-of-magnitude change.  There might also be other
    > approaches, like caching some planner settings in PlannerInfo.  Worst
    > case, as a transition measure, we could add assign hooks that write to
    > a global variable on a case-by-case basis.
    
    I'm sure it depends on the GUC. For most, I wouldn't be worried about 
    the cost. But I'm sure some are checked in critical loops currently.
    
    Instead of a hash table to hold the values, you could have a dynamically 
    extendable "struct". DefineCustomXXXVariable can reserve an offset and 
    store it in a global variable. So the code to read the current GUC value 
    would look something like this:
    
    /* defined elsewhere */
    struct Session {
    	...
    	/* this area holds all the GUC values for the current session */
    	char *guc_values;
    }
    
    _Thread_local struct Session *session;
    
    /* global variable set by DefineCustomBooleanVariable */
    size_t enable_seqscan_offset;
    
    void
    cost_seqscan(...)
    {
    	...
    	path->disabled_nodes = *(bool *) (session->gucs[seqscan_offset])  ? 0 : 1;
    	...
    }
    
    I'm imagining that we'd have some macros or helper functions to make 
    that look nicer in the calling code, but the above is what would happen 
    behind the scenes. So maybe the code would actually look like this:
    
    	...
    	path->disabled_nodes = get_config_val_bool(enable_seqscan) ? 0 : 1;
    	...
    
    
    I'd love the key to that function to be a pointer to a const struct or 
    something, instead of a string. That would allow some compile-time 
    checking that the GUC is actually defined and of the right type.
    
    
    PS. I found this blog post on how Thread Local Storage is implemented on 
    different systems very enlightening: 
    https://maskray.me/blog/2021-02-14-all-about-thread-local-storage. I 
    think whatever scheme we come up with will be a home-grown 
    implementation of one the methods described in that article.
    
    - Heikki
    
    
    
    
  3. Re: GUC thread-safety approaches

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-18T17:26:33Z

    On 18.11.25 15:15, Heikki Linnakangas wrote:
    > Instead of a hash table to hold the values, you could have a dynamically 
    > extendable "struct". DefineCustomXXXVariable can reserve an offset and 
    > store it in a global variable. So the code to read the current GUC value 
    > would look something like this:
    > 
    > /* defined elsewhere */
    > struct Session {
    >      ...
    >      /* this area holds all the GUC values for the current session */
    >      char *guc_values;
    > }
    > 
    > _Thread_local struct Session *session;
    > 
    > /* global variable set by DefineCustomBooleanVariable */
    > size_t enable_seqscan_offset;
    
    The way I understand this, this would only work if 
    DefineCustomXXXVariable could only be called from a global context 
    (e.g., shared_preload_libraries).  But AFAICT, you can define custom GUC 
    parameters per session (e.g., LOAD 'auto_explain'), and so each session 
    would have a different offset, and so the offset variable would itself 
    have to be session-local.
    
    
    
    
    
  4. Re: GUC thread-safety approaches

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-11-18T20:37:43Z

    On Tue, Nov 18, 2025, 11:26 Peter Eisentraut <peter@eisentraut.org> wrote:
    
    > The way I understand this, this would only work if
    > DefineCustomXXXVariable could only be called from a global context
    > (e.g., shared_preload_libraries).  But AFAICT, you can define custom GUC
    > parameters per session (e.g., LOAD 'auto_explain'), and so each session
    > would have a different offset, and so the offset variable would itself
    > have to be session-local.
    >
    
    I think that a session-local LOAD is something we're going to lose with
    threading anyway. A shared library is only going to be loaded once for the
    cluster, not once per backend. And to be clear: I think that's totally
    fine. (i.e. the benefit of these session-local extensions seems to small
    for us to find and maintain some workaround for this)
    
    So having a DefineCustomXXXVariable only be callable from a global context
    seems fine to me.
    
    I quite like the idea of this global offset array btw.
    
    >
    
  5. Re: GUC thread-safety approaches

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-11-18T20:52:44Z

    Jelte Fennema-Nio <postgres@jeltef.nl> writes:
    > I think that a session-local LOAD is something we're going to lose with
    > threading anyway. A shared library is only going to be loaded once for the
    > cluster, not once per backend.
    
    That's not necessarily true.  Certainly it will either be physically
    present in the address space or not, across the board.  But you could
    imagine that it has no effect in sessions that have not invoked its
    _PG_init function, because it hasn't been allowed to get into any
    hooks.  This would require that everything that _PG_init might touch
    be session-local state, which might not be terribly practical.  But
    if we wanted to preserve the current behavior, we could make that
    happen.
    
    > And to be clear: I think that's totally
    > fine. (i.e. the benefit of these session-local extensions seems to small
    > for us to find and maintain some workaround for this)
    
    I don't necessarily disagree with that.  Just saying that I don't
    think our hand is forced.
    
    			regards, tom lane
    
    
    
    
  6. Re: GUC thread-safety approaches

    Nico Williams <nico@cryptonector.com> — 2025-11-18T21:17:18Z

    On Tue, Nov 18, 2025 at 09:50:37AM +0100, Peter Eisentraut wrote:
    > I want to discuss possible approaches to making the GUC system
    > thread-safe.  In particular, I want to talk about the global
    > variables.
    
    I have a thing that might work for you, and OpenSSL has a better variant
    of that.  Basically user-land RCU.  Or something similar to Haskell
    MVars.  Or a concurrent hashmap.  Etc.  This wheel has been invented
    numerous times before.
    
    My thing is https://github.com/nicowilliams/ctp which gives you a
    "thread-safe global variable" with a `get()`, `release()`, `set()` API
    that gets you these semantics:
    
     - writes are serialized with a lock and will not wait forever for
       readers
    
     - reads (through a function) are fast and non-blocking
    
     - any value read is safe to keep using until either a) the thread
       releases it, or b) the thread reads the variable again
    
    Values published to this should be immutable or mostly immutable (you
    can mutate and access interior state with synchronized primitives, but
    you can't destroy the values themselves as they are destroyed
    automatically when the last reference is released).
    
    I originally wrote this to avoid needing dreadful read-write locking
    primitives to access global configuration state that changes very
    occasionally.  You'd use this to protect all GUCs together rather than
    one per-GUC.
    
    The OpenSSL variant is a hazard-pointer protected concurrent hashmap,
    and is in newer versions of OpenSSL.  OpenSSL used this to fix severe
    threaded performance issues they had in the 3.0 release.
    
    The OpenSSL variant would let you have high writer concurrency: N
    writers can concurrently change N different keys' values in the hashmap.
    This is a fantastic benefit that PG might not need.
    
    My advice would be to adapt the OpenSSL approach (licensing issues might
    force you to write your own), since it's more general than mine and has
    had more eyes, but then again I've been using mine in production for a
    decade.
    
    More importantly, I think you'll want primitives and data structures
    such as these for many other things besides GUCs.
    
    Cheers,
    
    Nico
    -- 
    
    
    
    
  7. Re: GUC thread-safety approaches

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-18T22:15:02Z

    On 18.11.25 15:15, Heikki Linnakangas wrote:
    > PS. I found this blog post on how Thread Local Storage is implemented on 
    > different systems very enlightening: https://maskray.me/blog/2021-02-14- 
    > all-about-thread-local-storage. I think whatever scheme we come up with 
    > will be a home-grown implementation of one the methods described in that 
    > article.
    
    I read this:
    
     > Windows TLS
    
     > Referencing a TLS variable from another DLL is not supported.
     >
     > __declspec(dllimport) extern thread_local int tls;
     > // error C2492: 'tls': data with thread storage duration may not have 
    dll interface
    
    
    So something like
    
    extern PGDLLIMPORT thread_local struct Port *MyProcPort;
    
    wouldn't work (confirmed).
    
    That might be a problem, since we now mark all global variables as 
    PGDLLIMPORT.
    
    
    
    
    
    
  8. Re: GUC thread-safety approaches

    David Rowley <dgrowleyml@gmail.com> — 2025-11-18T22:39:07Z

    On Tue, 18 Nov 2025 at 21:50, Peter Eisentraut <peter@eisentraut.org> wrote:
    > where get_config_val_*() would be a thin wrapper around hash_search()
    > (a bit like the existing GetConfigOption() and find_option(), but
    > without all the error checking).
    >
    > Would that be too expensive?
    
    Why couldn't in-core GUCs be fields in the Session struct and have a
    hash table for storage of custom GUCs, and allow core to access the
    fields directly? Extensions would need to go through a function which
    does the hash lookup.
    
    David
    
    
    
    
  9. Re: GUC thread-safety approaches

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-19T08:04:04Z

    On 18.11.25 23:39, David Rowley wrote:
    > On Tue, 18 Nov 2025 at 21:50, Peter Eisentraut <peter@eisentraut.org> wrote:
    >> where get_config_val_*() would be a thin wrapper around hash_search()
    >> (a bit like the existing GetConfigOption() and find_option(), but
    >> without all the error checking).
    >>
    >> Would that be too expensive?
    > 
    > Why couldn't in-core GUCs be fields in the Session struct and have a
    > hash table for storage of custom GUCs, and allow core to access the
    > fields directly? Extensions would need to go through a function which
    > does the hash lookup.
    
    Until now, we've made it so that in-core and custom GUCs behave exactly 
    the same, once defined.  Breaking that apart would create additional 
    complexity.  Also, as a general design principle in PostgreSQL, 
    extensions should have access to the same things as in-core code, and 
    in-core code should use the APIs provided to extensions.
    
    
    
    
    
  10. Re: GUC thread-safety approaches

    Matthias van de Meent <boekewurm+postgres@gmail.com> — 2025-11-19T19:16:07Z

    On Tue, 18 Nov 2025 at 09:50, Peter Eisentraut <peter@eisentraut.org> wrote:
    >
    > I want to discuss possible approaches to making the GUC system
    > thread-safe.  In particular, I want to talk about the global
    > variables.
    >
    > A GUC parameter is defined via a struct config_generic record that
    > contains some metadata and a pointer to a global variable.  For
    > example (simplified):
    >
    >      // elsewhere
    >      bool enable_seqscan = true;
    >
    >      // elsewhere
    >      extern bool enable_seqscan;
    >
    >      // in guc_tables.inc.c (generated)
    >      ...
    >      {
    >          .name = "enable_seqscan",
    >          .context = PGC_USERSET,
    >          .group = QUERY_TUNING_METHOD,
    >          .short_desc = gettext_noop("Enables the planner's use of
    > sequential-scan plans."),
    >          .flags = GUC_EXPLAIN,
    >          .vartype = PGC_BOOL,
    >          ._bool = {
    >              .variable = &enable_seqscan,  // HERE
    >              .boot_val = true,
    >          },
    >      },
    >
    > For a multithreaded server, one of the ideas thrown around was to
    > convert (most) global variables to thread-local variables.  That way,
    > the overall structure of the code could remain the same, and each
    > thread would see the same set of global variables as before.
    
    
    
    > So you could do
    >
    >      thread_local bool enable_seqscan = true;
    >
    > and
    >
    >      extern thread_local bool enable_seqscan;
    >
    > and as far as the code in optimizer/path/costsize.c or wherever is
    > concerned, it would work the same ways as before.
    >
    > But that doesn't work because:
    >
    > src/include/utils/guc_tables.inc.c:1617:37: error: initializer element
    > is not constant
    >   1617 |                         .variable = &enable_seqscan,
    >
    > Heikki had developed a workaround for this in his branch[0]: For each
    > GUC parameter, create a simple function that returns the address of the
    > variable, and the config_generic record stores the address of the
    > function.  So like this:
    >
    >      static bool *enable_seqscan_address(void) { return &enable_seqscan; }
    >
    >      {
    >          .name = "enable_seqscan",
    >          .context = PGC_USERSET,
    >          .group = QUERY_TUNING_METHOD,
    >          .short_desc = gettext_noop("Enables the planner's use of
    > sequential-scan plans."),
    >          .flags = GUC_EXPLAIN,
    >          .vartype = PGC_BOOL,
    >          ._bool = {
    >              .var_addr_func = enable_seqscan_address,  // HERE
    >              .boot_val = true,
    >          },
    >      },
    >
    > and then the code in guc.c that reads and sets the values is adjusted
    > like this in several places:
    >
    >      -                   *conf->variable = conf->reset_val;
    >      +                   *conf->var_addr_func() = conf->reset_val;
    >
    > This works.
    >
    > [0]: see https://wiki.postgresql.org/wiki/Multithreading
    >
    > Heikki's branch contains some macros to generate those helper
    > functions:
    >
    >      #define DEFINE_BOOL_GUC_ADDR(guc) \
    >         static bool *guc##_address(void) { return &guc; }
    >
    >      DEFINE_BOOL_GUC_ADDR(enable_seqscan)
    >
    > Note that this requires fixing up every GUC variable definition like
    > this.
    >
    > With the generated guc_tables.inc.c, we could now generate these
    > helper functions automatically.  But you'd still need to modify each
    > variable definition to add the thread_local specification.
    >
    > Actually, in Heikki's branch this is hidden behind macros and looks
    > like this:
    >
    >      session_guc bool enable_seqscan = true;
    >
    > And then there is additional tooling to check the annotations of all
    > global variables, GUC or not, like this.
    >
    > So with that approach, we could add these kinds of annotations first,
    > independent of thread support, and then later on add thread support
    > without any further global code changes.
    >
    > This, however, doesn't work for user-defined GUC parameters in
    > extensions.
    >
    > The interface for that looks like this:
    >
    >      DefineCustomBoolVariable("auto_explain.log_analyze",
    >                               "Use EXPLAIN ANALYZE for plan logging.",
    >                               NULL,
    >                               &auto_explain_log_analyze,  // pointer to
    > global var
    >                               false,
    >                               PGC_SUSET,
    >                               0,
    >                               NULL,
    >                               NULL,
    >                               NULL);
    >
    > In Heikki's branch, the signature of this and related functions are
    > changed like this:
    >
    >       extern void DefineCustomBoolVariable(const char *name,
    >                                           const char *short_desc,
    >                                           const char *long_desc,
    >      -                                    bool *valueAddr,
    >      +                                    GucBoolAddressHook addr_hook,
    >                                           bool bootValue,
    >                                           GucContext context,
    >                                           int flags,
    >
    > And then there are macros like shown earlier and some other ones to
    > define the required helper functions and hook this all together.
    >
    > As written, this would break source-code compatibility for all
    > extensions that use these functions.  We could conceivably create
    > alternative functions like DefineCustomBoolVariableExt() and make the
    > old interfaces wrappers around the new ones, or something like that.
    > But of course, we would ideally want extensions to adopt the new
    > system, whatever it might be, before long.
    >
    > The point is, while we could probably do this transition with
    > relatively little impact on the core code and built-in GUC parameters,
    > it appears that extension code will require nontrivial manual work to
    > adopt this and also maintain backward compatibility.  So we need to
    > think this through before shipping those interfaces.
    
    Agreed, we'll need to carefully consider this.
    
    > Now consider furthermore that in some future we might want to decouple
    > sessions from threads.  There is a lot of work to be done between here
    > and there, but it seems a quite plausible idea.  At that point, we
    > would need to get rid of the thread-local global variables anyway.  So
    > should we do that now already?  If we're going to force extension
    > authors to amend their code for this, can we do it so that they only
    > have to do it once?  It would be kind of annoying if one had to
    > support like three different custom-GUC interfaces in an extension
    > that wants to support five PostgreSQL major versions.
    
    
    
    > What might take the place of the global variables then?  Note that it
    > cannot just be a struct with fields for all the parameters, because
    > that's not extensible.  So it would need to be some kind of dynamic
    > key-value structure, like a hash table.  And we already have that!
    > All the GUC records are already in a hash table
    >
    >      static HTAB *guc_hashtab;
    >
    > which is used for all the utility commands and system views and so on.
    >
    > Could we use that for getting the current values at run time, too?
    >
    > So instead of
    >
    >      void
    >      cost_seqscan(...)
    >      {
    >          ...
    >          path->disabled_nodes = enable_seqscan ? 0 : 1;
    >          ...
    >      }
    >
    > do something like
    >
    >      void
    >      cost_seqscan(...)
    >      {
    >          ...
    >          path->disabled_nodes = get_config_val_bool("enable_seqscan") ?
    > 0 : 1;
    >          ...
    >      }
    >
    > where get_config_val_*() would be a thin wrapper around hash_search()
    > (a bit like the existing GetConfigOption() and find_option(), but
    > without all the error checking).
    
    I think this would be too expensive for extensions' GUCs, let alone
    our own GUCs.
    
    > Would that be too expensive?  This would have to be checked in detail,
    > of course, but just for this example I note that cost_seqscan() is not
    > afraid to do multiple hash table lookups anyway (e.g.,
    > get_tablespace_page_costs(), get_restriction_qual_cost()), so this
    > would not be an order-of-magnitude change.
    
    >   There might also be other
    > approaches, like caching some planner settings in PlannerInfo.  Worst
    > case, as a transition measure, we could add assign hooks that write to
    > a global variable on a case-by-case basis.
    
    
    
    > My question at this point is, which of these scenarios should we work
    > toward?  Either work toward thread-local variables and helper
    > functions and provide new APIs for extensions.  Or work toward getting
    > rid of the global variables and use hash-table lookups whenever the
    > value is needed, with some caching if necessary.  (Or other ideas?)
    
    I think we might generally benefit more from moving GUCs to a
    group-at-a-time (well, struct-at-a-time) approach.
    
    Right now, GUCs are individually addressed inside the GUC system, and
    the API is built for that. It's worked well for us, but for
    thread-local storage I don't think it'll scale - we'd store O(n*t)
    pointers if pointers are cached in thread-local state, and would
    definitely have to call each of these address-resolving functions
    during backend startup (when we setup the GUC tables with
    user/database session settings or are otherwise not pre-filled with
    their defaults, or for parallel workers restore GUC state from their
    leader). That's just not great - the amount of resolveable symbols for
    GUCs would need to double.
    
    I think we'd benefit from having each extension maintain a single GUC
    struct [^1] whose contents (GUC names, types, offsets in struct, other
    related checks) are registered with one call, together with a single
    TLS reference resolver function (like used in Heikki's design).
    This way, the GUC system only needs to keep track of a single TLS
    pointer function per registered set of GUCs, relaxing the overhead in
    GUC infra per guc by a few bytes (32 bits should be sufficient for GUC
    struct-internal offsets; possibly even 16 bits), and more easily
    allowing for caching the TLS pointers (it is assumed that a thread's
    TLS doesn't move). Extensions could link as usual, they'd just have to
    address the struct's fields instead of global variables.
    
    This would also have the benefit of making GUC state restoration
    faster through the use of memcpy/memmove on whole structs, rather than
    the current value-at-a-time approach. I believe that even in the
    current multiprocessing model we could benefit from this approach.
    
    Note: This would look similar to the "fields in a Session struct"
    approach from a GUC user's perspective, but it is more generalized to
    enable extensions to have a similar performance profile to native
    PostgreSQL.
    
    Maybe we could add headers to generate these structs from lists of
    GUCs like how rmgrlist.h works, or possibly have scripts to generate
    GUC tables from struct metadata like what gen_node_support.pl does for
    Node infrastructure? I suspect it would help making a transition
    easier.
    
    Kind regards,
    
    Matthias van de Meent
    
    [^1] for memory efficiency one per modification level would probably
    be better; PGC_POSTMASTER wouldn't change as much as SIGHUP or
    PGC_BACKEND, and therefore maybe shouldn't be co-located. Notably, PG
    itself would only need about 1.9kB to store all GUCs.