On Sat, 22 Aug 2026 at 01:47, Michael Paquier <[email protected]> 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 <[email protected]> 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)
v4-0002-Make-stringToNode-infrastructure-thread-safe.patch
Description: Binary data
v4-0001-Refactor-Move-read-functions-to-readfuncs.h.patch
Description: Binary data
