Thread-safe stringToNode() / pg_strtok()

On Fri, Aug 14, 2026 at 04:08:24PM +0200, Matthias van de Meent wrote:

In line with various other efforts making things more re-entrant and
thread-safe, here's a patch that makes stringToNode (and pg_strtok
with it) thread safe.

I have been reading the patch (not tested, just looking at the diffs),
and using a context as you are doing sounds like an acceptable thing
to do to remove the static pointer used by pg_strtok(). At least the
change makes sense here when taken in isolation.

There are other things we could do to improve this code (e.g.
de/serializaing based on node descriptors instead of generated
functions), but I consider such improvements to be out of scope for
this patch; the attached is just a simple move from global variables
to a context struct passed down the call stack.

That sounds fair.

Any thoughts or comments from others?
--
Michael

On Sun, 16 Aug 2026 at 09:13, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Aug 14, 2026 at 04:08:24PM +0200, Matthias van de Meent wrote:

In line with various other efforts making things more re-entrant and
thread-safe, here's a patch that makes stringToNode (and pg_strtok
with it) thread safe.

I have been reading the patch (not tested, just looking at the diffs),
and using a context as you are doing sounds like an acceptable thing
to do to remove the static pointer used by pg_strtok(). At least the
change makes sense here when taken in isolation.

Thanks for looking!

Any thoughts or comments from others?

One update for the patch. This fixes non-DEBUG_NODE_TESTS_ENABLED
builds; I forgot to update the READ_LOCATION_FIELD definition in those
non-debug builds, CI picked this up quite quickly but I didn't get to
fixing it until today.

-Matthias

Hi Matthias,

Thanks for v2. I think the ExtensibleNode path still needs an update.
_readExtensibleNode() receives ReadNodeContext *ctx, but still calls:
methods->nodeRead(local_node);

Since pg_strtok(), readBitmapset(), and the other reader helpers now
require ctx, an extension's nodeRead callback has no way to read its
private fields. External extensions such as Apache AGE, DocumentDB and
AQO implement ExtensibleNode nodeRead callbacks using pg_strtok() and
related reader helpers.

Should ExtensibleNodeMethods.nodeRead also receive ReadNodeContext, for
example:
void (*nodeRead)(ReadNodeContext *ctx, ExtensibleNode *node);
and be called as:
methods->nodeRead(ctx, local_node);

Best Regards,
Yuhang Qiu.

On 17.08.26 13:36, Matthias van de Meent wrote:

On Sun, 16 Aug 2026 at 09:13, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Aug 14, 2026 at 04:08:24PM +0200, Matthias van de Meent wrote:

In line with various other efforts making things more re-entrant and
thread-safe, here's a patch that makes stringToNode (and pg_strtok
with it) thread safe.

I have been reading the patch (not tested, just looking at the diffs),
and using a context as you are doing sounds like an acceptable thing
to do to remove the static pointer used by pg_strtok(). At least the
change makes sense here when taken in isolation.

Thanks for looking!

Any thoughts or comments from others?

One update for the patch. This fixes non-DEBUG_NODE_TESTS_ENABLED
builds; I forgot to update the READ_LOCATION_FIELD definition in those
non-debug builds, CI picked this up quite quickly but I didn't get to
fixing it until today.

The patch contains a comment:

+        * We don't have a separate type for const StringInfos, unconstify +
+        * careful coding will have to suffice.

But the patch doesn't contain any unconstify() calls.

On Wed, 19 Aug 2026 at 10:46, Peter Eisentraut <peter@eisentraut.org> wrote:

On 17.08.26 13:36, Matthias van de Meent wrote:

One update for the patch. This fixes non-DEBUG_NODE_TESTS_ENABLED
builds; I forgot to update the READ_LOCATION_FIELD definition in those
non-debug builds, CI picked this up quite quickly but I didn't get to
fixing it until today.

The patch contains a comment:

+        * We don't have a separate type for const StringInfos, unconstify +
+        * careful coding will have to suffice.

But the patch doesn't contain any unconstify() calls.

Thanks for noticing. An (unposted) earlier version stored a
StringInfoData, using its cursor field for decoding in pg_strtok(),
instead of changing the base string pointer.
The comment references the unconstify() needed to populate this
StringInfoData's non-const char* field.

Attached is patch v3, with new changes:
- Further simplified stringToNodeInternal;
- Another round of comments cleanup, including the one Peter pointed out;
- Added a small comment describing the ReadNodeContext struct;
- Fixed _readExtensibleNode()/ExtensibleNodeMethods->nodeRead(), per
Yuhang Qiu's review.

Kind regards,

Matthias van de Meent

Hi,

On 2026-08-16 16:13:20 +0900, Michael Paquier wrote:

On Fri, Aug 14, 2026 at 04:08:24PM +0200, Matthias van de Meent wrote:

There are other things we could do to improve this code (e.g.
de/serializaing based on node descriptors instead of generated
functions), but I consider such improvements to be out of scope for
this patch; the attached is just a simple move from global variables
to a context struct passed down the call stack.

That sounds fair.

Any thoughts or comments from others?

Tackling those things separately makes a lot of sense to me. Not using static
variables for the node infrastructure also makes a lof of sense, I find APIs
like pg_strtok() quite terrible, even disregarding thread safety.

Greetings,

Andres Freund

On Wed, Aug 19, 2026 at 03:13:42PM +0200, Matthias van de Meent wrote:

Thanks for noticing. An (unposted) earlier version stored a
StringInfoData, using its cursor field for decoding in pg_strtok(),
instead of changing the base string pointer.
The comment references the unconstify() needed to populate this
StringInfoData's non-const char* field.

Attached is patch v3, with new changes:
- Further simplified stringToNodeInternal;
- Another round of comments cleanup, including the one Peter pointed out;
- Added a small comment describing the ReadNodeContext struct;

I have looked at this patch in depth, and I like a lot what you are
doing here. Some comments available below.

A comment at the top of parseNodeString() says the following:
* The string to be read must already have been loaded into pg_strtok().

I believe that's not true anymore?

- Fixed _readExtensibleNode()/ExtensibleNodeMethods->nodeRead(), per
Yuhang Qiu's review.

I have never paid much attention to this file, but I think that this
points to another problem: RegisterExtensibleNodeMethods() is defined,
but we have zero caller of it in core. So there is a test gap for me
here. It's not the fault of your patch, and as far as I can see
yourchange looks fine, but it would be nice to close that coverage
hole.

Luckily enough, Aleksander Alekseev has posted a patch to add a test
module that would cover this API, and we could have node-level reads
and compares to even cover what you are doing here (some more work
still required):
/messages/by-id/CAJ7c6TNfn9Fv_Je1etA6rrgq1onVvXbjwBTkbkd4kVQhcu11gg@mail.gmail.com

Not something that we have to do here, but something I think you
should be made aware of. I am particularly interested in opinions
regarding possible gaps in what's proposed on the other thread.

-extern struct Bitmapset *readBitmapset(void);
-extern Datum readDatum(bool typbyval);
-extern bool *readBoolCols(int numCols);
-extern int *readIntCols(int numCols);
-extern Oid *readOidCols(int numCols);
-extern int16 *readAttrNumberCols(int numCols);
+extern struct Bitmapset *readBitmapset(ReadNodeContext *ctx);
+extern Datum readDatum(ReadNodeContext *ctx, bool typbyval);
+extern bool *readBoolCols(ReadNodeContext *ctx, int numCols);
+extern int *readIntCols(ReadNodeContext *ctx, int numCols);
+extern Oid *readOidCols(ReadNodeContext *ctx, int numCols);
+extern int16 *readAttrNumberCols(ReadNodeContext *ctx, int numCols);

Hmm. There is something that smells structurally wrong to me here.
Historically, readBitmapset() exists for out-of-core code, but I think
that based on what we are dealing with we should remove it and
encourage the use of readNode() instead. The bottom of readfuncs.c
lists four more of these functions, but readNode() would lead to a
similar result. My point is to get rid of the forward declaration of
ReadNodeContext in nodes.h, and do that as a preliminary patch. I
have quickly tested a move of these functions to readfuncs.c, making
them static, and that works. Let's also remove readBitmapset() and
recommend folks to modernize and move to readNode(). I'd suggest to
do that as a small refactoring piece done before the introduction of
the thread-safe pg_strtok().

And just to not sound suspicious, I have looked at
codesearch.debian.org to look at uses of these routines, and found
zero hits. So making them local to readfuncs.c to make the result of
this thread more elegant does not stress me much. :)

Similarly, it would be cleaner to have extensible.h include
nodes/readfuncs.h to grab the definition of ReadNodeContext.
--
Michael

On Fri, 21 Aug 2026 at 06:41, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, Aug 19, 2026 at 03:13:42PM +0200, Matthias van de Meent wrote:

Thanks for noticing. An (unposted) earlier version stored a
StringInfoData, using its cursor field for decoding in pg_strtok(),
instead of changing the base string pointer.
The comment references the unconstify() needed to populate this
StringInfoData's non-const char* field.

Attached is patch v3, with new changes:
- Further simplified stringToNodeInternal;
- Another round of comments cleanup, including the one Peter pointed out;
- Added a small comment describing the ReadNodeContext struct;

I have looked at this patch in depth, and I like a lot what you are
doing here. Some comments available below.

A comment at the top of parseNodeString() says the following:
* The string to be read must already have been loaded into pg_strtok().

I believe that's not true anymore?

Correct. I hadn't found this because "loaded into pg_strtok()" isn't exactly

-extern struct Bitmapset *readBitmapset(void);
-extern Datum readDatum(bool typbyval);
-extern bool *readBoolCols(int numCols);
-extern int *readIntCols(int numCols);
-extern Oid *readOidCols(int numCols);
-extern int16 *readAttrNumberCols(int numCols);
+extern struct Bitmapset *readBitmapset(ReadNodeContext *ctx);
+extern Datum readDatum(ReadNodeContext *ctx, bool typbyval);
+extern bool *readBoolCols(ReadNodeContext *ctx, int numCols);
+extern int *readIntCols(ReadNodeContext *ctx, int numCols);
+extern Oid *readOidCols(ReadNodeContext *ctx, int numCols);
+extern int16 *readAttrNumberCols(ReadNodeContext *ctx, int numCols);

Hmm. There is something that smells structurally wrong to me here.
Historically, readBitmapset() exists for out-of-core code, but I think
that based on what we are dealing with we should remove it and
encourage the use of readNode() instead.

I'm ambivalent about that. A direct call into the Bitmapset
(de)serializer avoids the overhead for dispatching to _readBitmapset()
in readNode().

Of all nodes, Bitmapset is the only special_read_write node type that
an ExtensibleNode implementation could reasonably want to serialize.
The other special_read_write node types (String, Integer, Float,
Boolean, Bitstring) are all parse node types, which I think are more
properly replaced with either an A_Const or the parsed primitive
values in custom planner/executor nodes.

My point is to get rid of the forward declaration of
ReadNodeContext in nodes.h, and do that as a preliminary patch.

ReadNodeContext is invented in this patchset with just a single patch.
What preliminary patch could be added that gets rid of something
introduced in later patches?

The bottom of readfuncs.c
lists four more of these functions, but readNode() would lead to a
similar result. [..] I
have quickly tested a move of these functions to readfuncs.c, making
them static, and that works.

If you refer to the read*Cols functions and/or readDatum, then your
conclusion is not accurate: The read*Cols functions don't operate on
Nodes, but on dense arrays of their respective non-node types. They
exist to help deserialize the various arrays or Datum values in a
node's fields.

A user could implement these functions by themselves, but exposing
these procedures helps avoid developers having to re-invent the wheel.
It's a simple wheel, sure, but a wheel it is regardless.

Let's also remove readBitmapset() and
recommend folks to modernize and move to readNode(). I'd suggest to
do that as a small refactoring piece done before the introduction of
the thread-safe pg_strtok().

And just to not sound suspicious, I have looked at
codesearch.debian.org to look at uses of these routines, and found
zero hits. So making them local to readfuncs.c to make the result of
this thread more elegant does not stress me much. :)

I'm happy to move the read*Cols, readBitmapset, and readDatum
declarations to readfuncs.h instead, if that's OK with you? I'd like
to keep them public for the reasons above - it's not like it costs us
a lot to maintain these.

Similarly, it would be cleaner to have extensible.h include
nodes/readfuncs.h to grab the definition of ReadNodeContext.

I don't think that everyone who needs to know the shape of
ExtensibleNode needs to know what a ReadNodeContext is. Using a
forward declaration for it makes more sense to me; especially given
that it currently also uses "struct ExtensibleNode" even though that
same header typedefs ExtensibleNode just a few lines above that.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Fri, Aug 21, 2026 at 05:36:23PM +0200, Matthias van de Meent wrote:

On Fri, 21 Aug 2026 at 06:41, Michael Paquier <michael@paquier.xyz> wrote:

Hmm. There is something that smells structurally wrong to me here.
Historically, readBitmapset() exists for out-of-core code, but I think
that based on what we are dealing with we should remove it and
encourage the use of readNode() instead.

I'm ambivalent about that. A direct call into the Bitmapset
(de)serializer avoids the overhead for dispatching to _readBitmapset()
in readNode().

Of all nodes, Bitmapset is the only special_read_write node type that
an ExtensibleNode implementation could reasonably want to serialize.
The other special_read_write node types (String, Integer, Float,
Boolean, Bitstring) are all parse node types, which I think are more
properly replaced with either an A_Const or the parsed primitive
values in custom planner/executor nodes.

Are you aware of code that relies on these specific calls? I have
poked as well at github, but could not find even one hit that refers
to non-forked code. I have seen traces of copies of this code, for
backward-compatibility purposes only (citus is one, due to its
branching model).

My point is to get rid of the forward declaration of
ReadNodeContext in nodes.h, and do that as a preliminary patch.

ReadNodeContext is invented in this patchset with just a single patch.
What preliminary patch could be added that gets rid of something
introduced in later patches?

I just mean to create a first patch to not publish anymore the
readBitmapset() & friends set of functions, make them static inside
readfuncs.c. This results in a simplification of your patch by not
requiring the knowledge of ReadNodeContext anymore in nodes.h and this
forward declaration. Sounds like a win to me based on my arguments of
the first paragraph, because it leads to a cleaner result.

If you refer to the read*Cols functions and/or readDatum, then your
conclusion is not accurate: The read*Cols functions don't operate on
Nodes, but on dense arrays of their respective non-node types. They
exist to help deserialize the various arrays or Datum values in a
node's fields.

A user could implement these functions by themselves, but exposing
these procedures helps avoid developers having to re-invent the wheel.
It's a simple wheel, sure, but a wheel it is regardless.

Argument that could also apply to a bunch of these. I am OK if
someone can point me out to some code out there where these are
relevants. In terms of in-core code and after searches in the open, I
just see no evidence that points to this direction..

I'm happy to move the read*Cols, readBitmapset, and readDatum
declarations to readfuncs.h instead, if that's OK with you? I'd like
to keep them public for the reasons above - it's not like it costs us
a lot to maintain these.

I'd still make the internals of node code leaner, I guess.. That
leads to a simpler result.
--
Michael

On Sat, 22 Aug 2026 at 01:47, Michael Paquier <michael@paquier.xyz> wrote:

On Fri, Aug 21, 2026 at 05:36:23PM +0200, Matthias van de Meent wrote:

On Fri, 21 Aug 2026 at 06:41, Michael Paquier <michael@paquier.xyz> wrote:

Hmm. There is something that smells structurally wrong to me here.
Historically, readBitmapset() exists for out-of-core code, but I think
that based on what we are dealing with we should remove it and
encourage the use of readNode() instead.

I'm ambivalent about that. A direct call into the Bitmapset
(de)serializer avoids the overhead for dispatching to _readBitmapset()
in readNode().

Of all nodes, Bitmapset is the only special_read_write node type that
an ExtensibleNode implementation could reasonably want to serialize.
The other special_read_write node types (String, Integer, Float,
Boolean, Bitstring) are all parse node types, which I think are more
properly replaced with either an A_Const or the parsed primitive
values in custom planner/executor nodes.

Are you aware of code that relies on these specific calls? I have
poked as well at github, but could not find even one hit that refers
to non-forked code. I have seen traces of copies of this code, for
backward-compatibility purposes only (citus is one, due to its
branching model).

Not for readBitmap, no. See below for others.

My point is to get rid of the forward declaration of
ReadNodeContext in nodes.h, and do that as a preliminary patch.

ReadNodeContext is invented in this patchset with just a single patch.
What preliminary patch could be added that gets rid of something
introduced in later patches?

I just mean to create a first patch to not publish anymore the
readBitmapset() & friends set of functions, make them static inside
readfuncs.c. This results in a simplification of your patch by not
requiring the knowledge of ReadNodeContext anymore in nodes.h and this
forward declaration. Sounds like a win to me based on my arguments of
the first paragraph, because it leads to a cleaner result.

I've moved them to readfuncs.h, which I think is a reasonable
compromise; avoiding the forward declaration whilst still exposing
these functions to extensions that might need them.

If you refer to the read*Cols functions and/or readDatum, then your
conclusion is not accurate: The read*Cols functions don't operate on
Nodes, but on dense arrays of their respective non-node types. They
exist to help deserialize the various arrays or Datum values in a
node's fields.

A user could implement these functions by themselves, but exposing
these procedures helps avoid developers having to re-invent the wheel.
It's a simple wheel, sure, but a wheel it is regardless.

Argument that could also apply to a bunch of these. I am OK if
someone can point me out to some code out there where these are
relevants. In terms of in-core code and after searches in the open, I
just see no evidence that points to this direction..

There's a "shardman" repo that seems to directly use readOidCols:
https://github.com/ZhaoDiankui/shardman/blob/merge/src/exchange.c#L162

Apache AGE seems to use (or at least, reference) our read*Cols
definitions in their backend/nodes/cypher_readfuncs.c; in copied
macros that don't currently seem to have any usage but might get used
in the future. Its READ_BITMAP_FIELD macro won't work as-is (it
references _readBitmap, which is the private/static version, whereas
readBitmap is the exposed version).

----------

Attached is version 4, which has these changes compared to v3:

* new 0001 patch, which moves the read* functions from nodes.h to readfuncs.h.
As was discussed upthread. 0002 is adjusted accordingly.
* nodes/extensible.h now includes nodes/readfuncs.h
I revised my opinion on this: I noticed my compiler complained
about the plain "struct ReadNodeContext *ctx" argument if I didn't put
a "struct ReadNodeContext;" declaration at the top level of the file,
which (when combined with relevant comments) would add more verbose
clutter than the alternative of including nodes/readfuncs.h.
* adjusted comment on parseNodeString, as reported upthread.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

On Sat, Aug 22, 2026 at 10:16:44PM +0200, Matthias van de Meent wrote:

I've moved them to readfuncs.h, which I think is a reasonable
compromise; avoiding the forward declaration whilst still exposing
these functions to extensions that might need them.

The forward declaration feeling unnecessary was my main grip, so I am
fine with the suggestion of moving them to readfuncs.c.

There's a "shardman" repo that seems to directly use readOidCols:
https://github.com/ZhaoDiankui/shardman/blob/merge/src/exchange.c#L162

Noted.

Apache AGE seems to use (or at least, reference) our read*Cols
definitions in their backend/nodes/cypher_readfuncs.c; in copied
macros that don't currently seem to have any usage but might get used
in the future. Its READ_BITMAP_FIELD macro won't work as-is (it
references _readBitmap, which is the private/static version, whereas
readBitmap is the exposed version).

I saw this one, but it just looked like a copy-paste of the
readfuncs.c code to me, not a legit call.

* new 0001 patch, which moves the read* functions from nodes.h to readfuncs.h.
As was discussed upthread. 0002 is adjusted accordingly.

OK for this one here.

* nodes/extensible.h now includes nodes/readfuncs.h
I revised my opinion on this: I noticed my compiler complained
about the plain "struct ReadNodeContext *ctx" argument if I didn't put
a "struct ReadNodeContext;" declaration at the top level of the file,
which (when combined with relevant comments) would add more verbose
clutter than the alternative of including nodes/readfuncs.h.

Avoiding the struct markers when we can is a better practice to me.
Digression: We still have a few places in the code where we have these
markers, I think, perhaps they could be cleaned up..

* adjusted comment on parseNodeString, as reported upthread.

Thanks.
--
Michael