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). >> 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
signature.asc
Description: PGP signature
