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

On Fri, 21 Aug 2026 at 06:41, Michael Paquier <michael(at)paquier(dot)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)