On Fri, 21 Aug 2026 at 06:41, Michael Paquier <[email protected]> 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)
