> On Wed, May 21, 2025 at 08:22:19PM GMT, Sami Imseih wrote:
> > > > At the same time AFAICT there isn't much more code paths
> > > > to worry about in case of a LocationExpr as a node
> > >
> > > I can imagine there are others like value expressions,
> > > row expressions, json array expressions, etc. that we may
> > > want to also normalize.
>
> > Exactly. When using a node, one can explicitly wrap whatever is needed
> > into it, while otherwise one would need to find a new way to piggy back
> > on A_Expr in a new context.
>
> Looking at the VALUES expression case, we will need to carry the info
> with SelectStmt and ultimately to RangeTblEntry which is where the
> values_list is, so either approach we take RangeTblEntry will need the
> LocationExpr pointer or the additional ParseLoc info I am suggesting.
> A_Expr is not used in the values list case.
Right, that's precisely my point -- introducing a new node will allow to
to use the same generalized mechanism in such scenarios as well, instead
of every time inventing something new.
> > I'll take a look at the proposed change, but a bit later.
>
> Here is a v4 to compare with v3.
>
> 0001- is the infrastructure to track the boundaries
> 0002- the changes to jumbling
Just to call this out, I don't think there is an agreement on squashing
Params, which you have added into 0002. Let's discuss this change
separately from the 18 open item.
---
Here is a short summary of the open item:
* An issue has been discovered with the squashing feature in 18, which
can lead to invalid normalized queries in pg_stat_statement.
* The proposed fix extends gram.y functionality capturing
the end location for list expressions to address that.
* There is a disagreement on how exactly to capture the location, the
options are introducing a new node LocationExpr or piggy back on an
existing A_Expr. I find the former more flexible and less invasive,
but looks like there are also other opinions.
Now, both flavour of the proposed solution could be still concidered too
invasive to be applied as a bug fix. I personally don't see it like
this, but I'm obviously biased. This leads us to following decisions to
be made:
* Is modifying parser (either adding a new node or modifying an existing
one) acceptable at this stage? I guess it would be enough to collect
couple of votes yes/no in this thread.
* If it's not acceptable, the feature could be reverted in 18, and the
fix could be applied to the master branch only.
I'm fine with both outcomes (apply the fix to both 18 and master, or
revert in 18 and apply the fix on master), and leave the decision to
Álvaro (sorry for causing all the troubles). It's fair to say that
reverting the feature will be the least risky move.
