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

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