Re: [multithreading] extension compatibility
On Thu, Jun 6, 2024 at 10:32 AM Heikki Linnakangas wrote: > > Well, OK, so it sounds like I'm outvoted, at least at the moment. > > Maybe that will change as more people vote, but for now, that's where > > we are. Given that, I suppose we want something more like Tristan's > > patch, but with a more extensible syntax. Does that sound right? > > +1 So, how shall we proceed here? Tristan, do you want to update your patch based on this feedback? -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
On 06/06/2024 17:23, Robert Haas wrote: On Thu, Jun 6, 2024 at 5:00 AM Heikki Linnakangas wrote: If there is some material harm from compiling with multithreading support even if you're not using it, we should try to fix that. I'm not dead set against having a compile-time option, but I don't see the need for it at the moment. Well, OK, so it sounds like I'm outvoted, at least at the moment. Maybe that will change as more people vote, but for now, that's where we are. Given that, I suppose we want something more like Tristan's patch, but with a more extensible syntax. Does that sound right? +1 -- Heikki Linnakangas Neon (https://neon.tech)
Re: [multithreading] extension compatibility
On Thu, Jun 6, 2024 at 5:00 AM Heikki Linnakangas wrote: > If there is some material harm from compiling with multithreading > support even if you're not using it, we should try to fix that. I'm not > dead set against having a compile-time option, but I don't see the need > for it at the moment. Well, OK, so it sounds like I'm outvoted, at least at the moment. Maybe that will change as more people vote, but for now, that's where we are. Given that, I suppose we want something more like Tristan's patch, but with a more extensible syntax. Does that sound right? -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
On 06/06/2024 05:47, Robert Haas wrote: On Wed, Jun 5, 2024 at 10:09 PM Andres Freund wrote: Maybe. I think shipping a mode where users can fairly simply toggle between threaded and process mode will allow us to get this stable a *lot* quicker than if we distribute two builds. Most users don't build from source, distros will have to pick the mode. If they don't choose threaded mode, we'll not find problems. If they do choose threaded mode, we can't ask users to switch to a process based mode to check if the problem is related. I don't believe that being coercive here is the right approach. I think distros see the value in compiling with as many things turned on as possible; when they ship with something turned off, it's because it's unstable or introduces weird dependencies or has some other disadvantage. I presume there's no harm in building with multithreading support. If you don't want to use it, put "multithreading=off" in your config file (which will presumably be the default for a while). If we're worried about the performance impact of thread-local variables in particular, we can try to measure that. I don't think it's material though. If there is some material harm from compiling with multithreading support even if you're not using it, we should try to fix that. I'm not dead set against having a compile-time option, but I don't see the need for it at the moment. -- Heikki Linnakangas Neon (https://neon.tech)
Re: [multithreading] extension compatibility
On Wed, Jun 5, 2024 at 10:09 PM Andres Freund wrote: > Maybe. I think shipping a mode where users can fairly simply toggle between > threaded and process mode will allow us to get this stable a *lot* quicker > than if we distribute two builds. Most users don't build from source, distros > will have to pick the mode. If they don't choose threaded mode, we'll not find > problems. If they do choose threaded mode, we can't ask users to switch to a > process based mode to check if the problem is related. I don't believe that being coercive here is the right approach. I think distros see the value in compiling with as many things turned on as possible; when they ship with something turned off, it's because it's unstable or introduces weird dependencies or has some other disadvantage. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
Hi, On 2024-06-05 21:59:42 -0400, Robert Haas wrote: > On Wed, Jun 5, 2024 at 9:50 PM Andres Freund wrote: > > Depending on the architecture / ABI / compiler options it's often not > > meaningfully more expensive to access a thread local variable than a > > "normal" > > variable. > > > > I think these days it's e.g. more expensive on x86-64 windows, but not > > linux. On arm the overhead of TLS is more noticeable, across platforms, > > afaict. > > I mean, to me, this still sounds like we want multithreading to be a > build-time option. Maybe. I think shipping a mode where users can fairly simply toggle between threaded and process mode will allow us to get this stable a *lot* quicker than if we distribute two builds. Most users don't build from source, distros will have to pick the mode. If they don't choose threaded mode, we'll not find problems. If they do choose threaded mode, we can't ask users to switch to a process based mode to check if the problem is related. We have been talking in a bunch of threads about having a mode where the main postgres binary chooses a build optimized for the current architecture, to be able to use SIMD instructions without a runtime check/dispatch. I guess we could add threadedness to such a matrix... Greetings, Andres Freund
Re: [multithreading] extension compatibility
On Wed, Jun 5, 2024 at 9:50 PM Andres Freund wrote: > Depending on the architecture / ABI / compiler options it's often not > meaningfully more expensive to access a thread local variable than a "normal" > variable. > > I think these days it's e.g. more expensive on x86-64 windows, but not > linux. On arm the overhead of TLS is more noticeable, across platforms, > afaict. I mean, to me, this still sounds like we want multithreading to be a build-time option. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
Hi, On 2024-06-05 21:10:01 -0400, Robert Haas wrote: > On Wed, Jun 5, 2024 at 8:01 PM Heikki Linnakangas wrote: > > I'm very much in favor of a runtime toggle. To be precise, a > > PGC_POSTMASTER setting. We'll get a lot more testing if you can easily > > turn it on/off, and so far I haven't seen anything that would require it > > to be a compile time option. > > I was thinking about global variable annotations. If someone wants to > build without multithreading, I think that they won't want to still > end up with a ton of variables being changed to thread-local. Depending on the architecture / ABI / compiler options it's often not meaningfully more expensive to access a thread local variable than a "normal" variable. I think these days it's e.g. more expensive on x86-64 windows, but not linux. On arm the overhead of TLS is more noticeable, across platforms, afaict. Example compiler output for x86-64 and armv8: https://godbolt.org/z/K369eG5MM Cycle analysis or linux x86-64 output: https://godbolt.org/z/KK57vM1of This shows that for the linux x86-64 case there's no difference in efficiency between the tls/non-tls case. The reason it's so fast on x86-64 linux is that they reused one of the "old" segment registers to serve as the index register differing between each thread. For x86-64 code, most code is compiled position independent, and *also* uses an indexed mode (but relative to the instruction pointer). I think we might be able to gain some small performance benefits via the annotations, which actualy might make it viable to just apply the annotations regardless of using threads or not. Greetings, Andres Freund
Re: [multithreading] extension compatibility
On Wed, Jun 5, 2024 at 8:01 PM Heikki Linnakangas wrote: > I'm very much in favor of a runtime toggle. To be precise, a > PGC_POSTMASTER setting. We'll get a lot more testing if you can easily > turn it on/off, and so far I haven't seen anything that would require it > to be a compile time option. I was thinking about global variable annotations. If someone wants to build without multithreading, I think that they won't want to still end up with a ton of variables being changed to thread-local. So I think there has to be a build-time option controlling whether this build supports threading. I suspect there will be other people who want to just shut all of this experimental code off, which is probably going to be a second driver for a build-time toggle. But even with that, we can still have a GUC controlling whether threading is actually used. Does that make sense to you? Supposing it does, then how does the extension-marking system need to work? I suppose in this world we don't want any build failures: you're allowed to build a non-thread-aware extension against a threading-capable PostgreSQL; you're just not allowed to load the resulting extension when the server is in threading mode. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
On 05/06/2024 23:55, Robert Haas wrote: On Wed, Jun 5, 2024 at 4:32 PM Tristan Partin wrote: Not entirely sure how I feel about the approach you've taken, but here is a patch that Heikki and I put together for extension compatibility. It's not a build time solution, but a runtime solution. Instead of PG_MODULE_MAGIC, extensions would use PG_MAGIC_MODULE_REENTRANT. There is a GUC called `multithreaded` which controls the variable IsMultithreaded. We operated under the assumption that being able to toggle multithreading and multi-processing without recompiling has value. That's interesting, because I thought Heikki was against having a runtime toggle. I'm very much in favor of a runtime toggle. To be precise, a PGC_POSTMASTER setting. We'll get a lot more testing if you can easily turn it on/off, and so far I haven't seen anything that would require it to be a compile time option. I don't think PG_MODULE_MAGIC_REENTRANT is a good syntax. It all looks great as long as we only ever need the PG_MODULE_MAGIC line to signal one bit of information, but as soon as you get to two bits it's pretty questionable, and anything more than two bits is insane. If we want to do something with the PG_MODULE_MAGIC line, I think it should involve options-passing of some form rather than just having an alternate macro name. +1, that would be nicer. -- Heikki Linnakangas Neon (https://neon.tech)
Re: [multithreading] extension compatibility
On Wed Jun 5, 2024 at 3:56 PM CDT, Robert Haas wrote: On Wed, Jun 5, 2024 at 4:32 PM Tristan Partin wrote: > Not entirely sure how I feel about the approach you've taken, but here > is a patch that Heikki and I put together for extension compatibility. > It's not a build time solution, but a runtime solution. Instead of > PG_MODULE_MAGIC, extensions would use PG_MAGIC_MODULE_REENTRANT. There > is a GUC called `multithreaded` which controls the variable > IsMultithreaded. We operated under the assumption that being able to > toggle multithreading and multi-processing without recompiling has > value. That's interesting, because I thought Heikki was against having a runtime toggle. I don't think PG_MODULE_MAGIC_REENTRANT is a good syntax. It all looks great as long as we only ever need the PG_MODULE_MAGIC line to signal one bit of information, but as soon as you get to two bits it's pretty questionable, and anything more than two bits is insane. If we want to do something with the PG_MODULE_MAGIC line, I think it should involve options-passing of some form rather than just having an alternate macro name. I agree that this method doesn't lend itself to future extensibility. -- Tristan Partin https://tristan.partin.io
Re: [multithreading] extension compatibility
On Wed, Jun 5, 2024 at 4:32 PM Tristan Partin wrote: > Not entirely sure how I feel about the approach you've taken, but here > is a patch that Heikki and I put together for extension compatibility. > It's not a build time solution, but a runtime solution. Instead of > PG_MODULE_MAGIC, extensions would use PG_MAGIC_MODULE_REENTRANT. There > is a GUC called `multithreaded` which controls the variable > IsMultithreaded. We operated under the assumption that being able to > toggle multithreading and multi-processing without recompiling has > value. That's interesting, because I thought Heikki was against having a runtime toggle. I don't think PG_MODULE_MAGIC_REENTRANT is a good syntax. It all looks great as long as we only ever need the PG_MODULE_MAGIC line to signal one bit of information, but as soon as you get to two bits it's pretty questionable, and anything more than two bits is insane. If we want to do something with the PG_MODULE_MAGIC line, I think it should involve options-passing of some form rather than just having an alternate macro name. -- Robert Haas EDB: http://www.enterprisedb.com
Re: [multithreading] extension compatibility
On Wed, 5 Jun 2024 at 22:05, Robert Haas wrote: > The attached patch is a sketch of one possible approach: PostgreSQL > signals whether it is multithreaded by defining or not defining > PG_MULTITHREADING in pg_config_manual.h, and an extension signals > thread-readiness by defining PG_THREADSAFE_EXTENSION before including > any PostgreSQL headers other than postgres.h. My first gut-reaction: It seems kinda annoying to have to do this for every c that you use to build your extension, e.g. citus or postgis have a ton of those. PG_MODULE_MAGIC seems like a better fit imho. If we really want a compile time failure, then I think I'd prefer to have a new postgres.h file (e.g. postgres-thread.h) that you would include instead of plain postgres.h. Basically this file could then contain the below two lines: #include "postgres.h" #define PG_THREADSAFE_EXTENSION1
Re: [multithreading] extension compatibility
On Wed Jun 5, 2024 at 3:05 PM CDT, Robert Haas wrote:
...
== Extension Compatibility Solutions ==
The attached patch is a sketch of one possible approach: PostgreSQL
signals whether it is multithreaded by defining or not defining
PG_MULTITHREADING in pg_config_manual.h, and an extension signals
thread-readiness by defining PG_THREADSAFE_EXTENSION before including
any PostgreSQL headers other than postgres.h. If PostgreSQL is built
multithreaded and the extension does not signal thread-safety, you get
something like this:
../pgsql/src/test/modules/dummy_seclabel/dummy_seclabel.c:20:1: error:
static assertion failed due to requirement '1 == 0': must define
PG_THREADSAFE_EXTENSION or use unthreaded PostgreSQL
PG_MODULE_MAGIC;
I'm not entirely happy with this solution because the results are
confusing if PG_THREADSAFE_EXTENSION is declared after including
fmgr.h. Perhaps this can be adequately handled by documenting and
demonstrating the right pattern, or maybe somebody has a better idea.
Another idea I considered was to replace the PG_MODULE_MAGIC;
declaration with something that allows for arguments, like
PG_MODULE_MAGIC(.process_model = false, .thread_model = true). But on
further reflection, that seems like the wrong thing. AFAICS, that's
going to tell you at runtime about something that you really want to
know at compile time. But this kind of idea might need more thought if
we decide that the *same* build of PostgreSQL can either launch
processes or threads per session, because then we'd to know which
extensions were available in whichever mode applied to the current
session.
Not entirely sure how I feel about the approach you've taken, but here
is a patch that Heikki and I put together for extension compatibility.
It's not a build time solution, but a runtime solution. Instead of
PG_MODULE_MAGIC, extensions would use PG_MAGIC_MODULE_REENTRANT. There
is a GUC called `multithreaded` which controls the variable
IsMultithreaded. We operated under the assumption that being able to
toggle multithreading and multi-processing without recompiling has
value.
--
Tristan Partin
https://tristan.partin.io
From 60b0225d8e82d412237297710a7f007b006a7773 Mon Sep 17 00:00:00 2001
From: Tristan Partin
Date: Thu, 31 Aug 2023 11:08:01 -0500
Subject: [PATCH] Add PG_MODULE_MAGIC_REENTRANT
Extensions should use this if and only if they support reentrancy. If
the multithreaded GUC is set to true, then we can only load extensions
which support reentrancy.
---
src/backend/utils/fmgr/dfmgr.c | 14 +++---
src/include/fmgr.h | 23 +++
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 6ee9053044917..c2e6c22127067 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -85,7 +85,7 @@ static char *substitute_libpath_macro(const char *name);
static char *find_in_dynamic_libpath(const char *basename);
/* Magic structure that module needs to match to be accepted */
-static static_singleton const Pg_magic_struct magic_data = PG_MODULE_MAGIC_DATA;
+static static_singleton const Pg_magic_struct magic_data = PG_MODULE_MAGIC_REENTRANT_DATA;
/*
@@ -255,8 +255,9 @@ internal_load_library(const char *libname)
{
const Pg_magic_struct *magic_data_ptr = (*magic_func) ();
+ /* We will check reentrancy later. */
if (magic_data_ptr->len != magic_data.len ||
-memcmp(magic_data_ptr, _data, magic_data.len) != 0)
+memcmp(magic_data_ptr, _data, offsetof(Pg_magic_struct, reentrant)) != 0)
{
/* copy data block before unlinking library */
Pg_magic_struct module_magic_data = *magic_data_ptr;
@@ -268,6 +269,13 @@ internal_load_library(const char *libname)
/* issue suitable complaint */
incompatible_module_error(libname, _magic_data);
}
+
+ if (IsMultiThreaded && !magic_data_ptr->reentrant)
+ereport(ERROR,
+ (errmsg("incompatible library \"%s\": not reentrant",
+libname),
+errhint("Extension libraries must use the PG_MODULE_MAGIC_REENTRANT macro to be loaded "
+ "when multithreading is turned on.")));
}
else
{
@@ -278,7 +286,7 @@ internal_load_library(const char *libname)
ereport(ERROR,
(errmsg("incompatible library \"%s\": missing magic block",
libname),
- errhint("Extension libraries are required to use the PG_MODULE_MAGIC macro.")));
+ errhint("Extension libraries are required to use the PG_MODULE_MAGIC or PG_MODULE_MAGIC_REENTRANT macros.")));
}
/*
diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index 3ecb98a907d33..b49901eacae10 100644
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -470,6 +470,7 @@ typedef struct
int namedatalen; /* NAMEDATALEN */
int float8byval; /* FLOAT8PASSBYVAL */
char abi_extra[32]; /* see pg_config_manual.h */
+ bool reentrant; /* Whether the extension supports reentrancy */
} Pg_magic_struct;
/* The
