On Mon, 11 Mar 2024 at 14:19, Peter Eisentraut <peter@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+postgres@gmail.com> wrote:
> >>
> >> On Mon, 19 Feb 2024 at 14:19, Matthias van de Meent
> >> <boekewurm+postgres@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
