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

On Sat, 22 Aug 2026 at 01:47, Michael Paquier <michael(at)paquier(dot)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(at)paquier(dot)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)