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? > - Fixed _readExtensibleNode()/ExtensibleNodeMethods->nodeRead(), per > Yuhang Qiu's review. I have never paid much attention to this file, but I think that this points to another problem: RegisterExtensibleNodeMethods() is defined, but we have zero caller of it in core. So there is a test gap for me here. It's not the fault of your patch, and as far as I can see yourchange looks fine, but it would be nice to close that coverage hole. Luckily enough, Aleksander Alekseev has posted a patch to add a test module that would cover this API, and we could have node-level reads and compares to even cover what you are doing here (some more work still required): https://www.postgresql.org/message-id/caj7c6tnfn9fv_je1eta6rrgq1onvvxbjwbtkbkd4kvqhcu1...@mail.gmail.com Not something that we have to do here, but something I think you should be made aware of. I am particularly interested in opinions regarding possible gaps in what's proposed on the other thread. -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. The bottom of readfuncs.c lists four more of these functions, but readNode() would lead to a similar result. My point is to get rid of the forward declaration of ReadNodeContext in nodes.h, and do that as a preliminary patch. I have quickly tested a move of these functions to readfuncs.c, making them static, and that works. 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. :) Similarly, it would be cleaner to have extensible.h include nodes/readfuncs.h to grab the definition of ReadNodeContext. -- Michael
signature.asc
Description: PGP signature
