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
