On Mon, 11 Mar 2024 at 14:19, Peter Eisentraut <pe...@eisentraut.org> wrote: > > On 22.02.24 16:07, Matthias van de Meent wrote: > > On Thu, 22 Feb 2024 at 13:37, Matthias van de Meent > > <boekewurm+postg...@gmail.com> wrote: > >> > >> On Mon, 19 Feb 2024 at 14:19, Matthias van de Meent > >> <boekewurm+postg...@gmail.com> wrote: > >>> Attached the updated version of the patch on top of 5497daf3, which > >>> incorporates this last round of feedback. > >> > >> Now attached rebased on top of 93db6cbd to fix conflicts with fbc93b8b > >> and an issue in the previous patchset: I attached one too many v3-0001 > >> from a previous patch I worked on. > > > > ... and now with a fix for not overwriting newly deserialized location > > attributes with -1, which breaks test output for > > READ_WRITE_PARSE_PLAN_TREES installations. Again, no other significant > > changes since the patch of last Monday. > > * v7-0002-pg_node_tree-Don-t-store-query-text-locations-in-.patch > > This patch looks much more complicated than I was expecting. I had > suggested to model this after stringToNodeWithLocations(). This uses a > global variable to toggle the mode. Your patch creates a function > nodeToStringNoQLocs() -- why the different naming scheme?
It doesn't just exclude .location fields, but also Query.len, a similar field which contains the length of the query's string. The name has been further refined to nodeToStringNoParseLocs() in the attached version, but feel free to replace the names in the patch to anything else you might want. > -- and passes > the flag down as an argument to all the output functions. I mean, in a > green field, avoiding global variables can be sensible, of course, but I > think in this limited scope here it would really be better to keep the > code for the two directions read and write the same. I'm a big fan of _not_ using magic global variables as passed context without resetting on subnormal exits... For GUCs their usage is understandable (and there is infrastructure to reset them, and you're not supposed to manually update them), but IMO here its usage should be a function-scoped variable or in a passed-by-reference context struct, not a file-local static. Regardless, attached is an adapted version with the file-local variable implementation. > Attached is a small patch that shows what I had in mind. (It doesn't > contain any callers, but your patch shows where all those would go.) Attached a revised version that does it like stringToNodeInternal's handling of restore_location_fields. > * v7-0003-gen_node_support.pl-Mark-location-fields-as-type-.patch > > This looks sensible, but maybe making Location a global type is a bit > much? Maybe something more specific like ParseLocation, or ParseLoc, to > keep it under 12 characters. I've gone with ParseLoc in the attached v8 patchset. Kind regards, Matthias van de Meent
v8-0003-pg_node_tree-Don-t-store-query-text-locations-in-.patch
Description: Binary data
v8-0001-pg_node_tree-Omit-serialization-of-fields-with-de.patch
Description: Binary data
v8-0005-nodeToString-omit-serializing-NULL-datums-in-Cons.patch
Description: Binary data
v8-0004-gen_node_support.pl-Add-a-TypMod-type-for-signall.patch
Description: Binary data
v8-0002-gen_node_support.pl-Mark-location-fields-as-type-.patch
Description: Binary data
v8-0006-nodeToString-Apply-RLE-on-Bitmapset-and-numeric-L.patch
Description: Binary data
v8-0008-nodeToString-omit-serializing-0s-in-enum-typed-fi.patch
Description: Binary data
v8-0007-gen_node_support.pl-Optimize-serialization-of-fie.patch
Description: Binary data
