On 17.01.23 04:48, Michael Paquier wrote:
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.
Because we don't want to track the location of all the nodes! If we
do that, pg_stat_statements would begin to parameterize a lot more
things, from column aliases to full contents of IN or WITH clauses.
The root point is that we only want to track the jumble location for
Const nodes now. In order to do that, there are two approaches:
- Mark all the locations with jumble_ignore: more invasive, but
it requires only one per-field attribute with "jumble_ignore". This
is what v1 did.
Ok, I understand now, and I agree with this approach over the opposite.
I was confused because the snippet you showed above used
"jumble_ignore", but your patch is correct as it uses "jumble_location".
That said, the term "jumble" is really weird, because in the sense that
we are using it here it means, approximately, "to mix together", "to
unify". So what we are doing with the Const nodes is really to *not*
jumble the location, but for all other node types we are jumbling the
location. At least that is my understanding.
