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

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).

>> 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