Re: Generating code for query jumbling through gen_node_support.pl

On 13.01.23 08:54, Michael Paquier wrote:
> Using a "jumble_ignore" flag is equally invasive to using an
> "jumble_include" flag for each field, I am afraid, as the number of
> fields in the nodes included in jumbles is pretty equivalent to the
> number of fields ignored.  I tend to prefer the approach of ignoring
> things though, which is more consistent with the practive for node
> read, write and copy.

I concur that jumble_ignore is better.  I suppose you placed the 
jumble_ignore markers to maintain parity with the existing code, but I 
think that some the markers are actually wrong and are just errors of 
omission in the existing code (such as Query.override, to pick a random 
example).  The way you have structured this would allow us to find and 
analyze such errors better.

> Anyway, when it comes to the location, another thing that can be
> considered here would be to require a node-level flag for the nodes on
> which we want to track the location.  This overlaps a bit with the
> fields satisfying "($t eq 'int' && $f =~ 'location$')", but it removes
> most of the code changes like this one as at the end we only care
> about the location for Const nodes:
> -   int         location;       /* token location, or -1 if unknown */
> +   int         location pg_node_attr(jumble_ignore);   /* token location, or -1
> +                                                        * if unknown */
> 
> I have taken this approach in v2 of the patch, shaving ~100 lines of
> more code as there is no need to mark all these location fields with
> "jumble_ignore" anymore, except for Const, of course.

I don't understand why you chose that when we already have an 
established way.  This would just make the jumble annotations 
inconsistent with the other annotations.

I also have two suggestions to make this patch more palatable:

1. Make a separate patch to reformat long comments, like 
835d476fd21bcfb60b055941dee8c3d9559af14c.

2. Make a separate patch to move the jumble support to 
src/backend/nodes/jumblefuncs.c.  (It was probably a mistake that it 
wasn't there to begin with, and some of the errors of omission alluded 
to above are probably caused by it having been hidden away in the wrong 
place.)