Re: Thread-safe stringToNode() / pg_strtok()

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):
https://www.postgresql.org/message-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