I spent a few hours looking into this today and to your points below:
> > I agree that the current solution we have in the tree feels incomplete
> > because we are not taking into account the most common cases that
> > users would care about. Now, allowing PARAM_EXTERN means that we
> > allow any expression to be detected as a squashable thing, and this
> > kinds of breaks the assumption of IsSquashableConst() where we want
> > only constants to be allowed because EXECUTE parameters can be any
> > kind of Expr nodes. At least that's the intention of the code on
> > HEAD.
> >
> > Now, I am not actually objecting about PARAM_EXTERN included or not if
> > there's a consensus behind it and my arguments are considered as not
> > relevant. The patch is written so as it claims that a PARAM_EXTERN
> > implies the expression to be a Const, but it may not be so depending
> > on what the execution path is given for the parameter. Or at least
> > the patch could be clearer and rename the parts about the "Const"
> > squashable APIs around queryjumblefuncs.c.
> >
> > [...]
> >
> > The PARAM_EXTERN part has been mentioned a couple of weeks ago here,
> > btw:
> > https://www.postgresql.org/message-id/CAA5RZ0tu6_KRiYJCFptf4_--wjFSu9cZMj1XNmOCqTNxu=v...@mail.gmail.com
>
> In fact, this has been discussed much earlier in the thread above, as
> essentially the same implementation with T_Params, which is submitted
> here, was part of the original patch. The concern was always whether or
> not it will break any assumption about query identification, because
> this way much broader scope of expressions will be considered equivalent
> for query id computation purposes.
>
> At the same time after thinking about this concern more, I presume it
> already happens at a smaller scale -- when two queries happen to have
> the same number of parameters, they will be indistinguishable even if
> parameters are different in some way.
I don't think limiting this feature to Const only will suffice.
I think what we should really allow the broader scope of expressions that
are allowed via prepared statements, and this will make this implementation
consistent between prepared vs non-prepared statements. I don't see why
not. In fact, when we are examining the ArrayExpr, I think the only
thing we should
not squash is if we find a Sublink ( i.e. SELECT statement inside the array ).
> > To be honest, the situation of HEAD makes me question whether we are
> > using the right approach for this facility. I did mention a couple of
> > months ago about an alternative, but it comes down to accept that any
> > expressions would be normalized, unfortunately I never got down to
> > study it in details as this touches around expr_list in the parser: we
> > could detect in the parser the start and end locations of an
> > expression list in a query string, then group all of them together
> > based on their location in the string. This would be also cheaper
> > than going through all the elements in the list, tweaking things when
> > dealing with a subquery..
>
> Not entirely sure how that would work exactly, but after my experiments
> with the squashing patch I found it could be very useful to be able to
> identify the end location of an expression list in the parser.
I also came to the same conclusion, that we should track the start '('
and end ')'
location of a expression list to allow us to hide the fields. But, I
will look into
other approaches as well.
I am really leaning towards that we should revert this feature as the
limitation we have now with parameters is a rather large one and I think
we need to go back and address this issue.
--
Sami
