Re: Reducing output size of nodeToString

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.