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