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? -- 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.
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.)
* 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.