Re: Schema variables - new implementation for Postgres 15

st 8. 3. 2023 v 16:35 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
napsal:

> > On Wed, Mar 08, 2023 at 08:31:07AM +0100, Pavel Stehule wrote:
> > pá 3. 3. 2023 v 21:19 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> > napsal:
> >
> > > > On Tue, Feb 28, 2023 at 06:12:50AM +0100, Pavel Stehule wrote:
> > > >
> > > > fresh rebase
> > >
> > > I'm continuing to review, this time going through shadowing stuff in
> > > transformColumnRef, IdentifyVariable etc. Well, that's a lot of leg
> work
> > > for rather little outcome :) I guess all attempts to simplify this part
> > > weren't successful?
> > >
> >
> > Originally I wrote it in the strategy "reduce false alarms". But when I
> > think about it, it may not be good in this case. Usually the changes are
> > done first on some developer environment, and good practice is to
> disallow
> > same (possibly confusing) identifiers. So I am not against making this
> > warning more aggressive with some possibility of false alarms. With
> > blocking reduction of alarms the differences in regress was zero. So I
> > reduced this part.
>
> Great, thank you.
>
> > > Couple of questions to it. In IdentifyVariable in the branch handling
> > > two values the commentary says:
> > >
> > > /*
> > > * a.b can mean "schema"."variable" or "variable"."field",
> > > * Check both variants, and returns InvalidOid with
> > > * not_unique flag, when both interpretations are
> > > * possible. Second node can be star. In this case, the
> > > * only allowed possibility is "variable"."*".
> > > */
> > >
> > > I read this as "variable"."*" is a valid combination, but the very next
> > > part of this condition says differently:
> > >
> >
> >
> >
> > >
> > > /*
> > > * Session variables doesn't support unboxing by star
> > > * syntax. But this syntax have to be calculated here,
> > > * because can come from non session variables related
> > > * expressions.
> > > */
> > > Assert(IsA(field2, A_Star));
> > >
> > > Is the first commentary not quite correct?
> > >
> >
> > I think it is correct, but maybe I was not good at describing this issue.
> > The sentence "Second node can be a star. In this case, the
> > the only allowed possibility is "variable"."*"." should be in the next
> > comment.
> >
> > In this part we process a list of identifiers, and we try to map these
> > identifiers to some semantics. The parser should ensure that
> > all fields of lists are strings or the last field is a star. In this case
> > the semantic "schema".* is nonsense, and the only possible semantic
> > is "variable".*. It is valid semantics, but unsupported now. Unboxing is
> > available by syntax (var).*
> >
> > I changed the comment
>
> Thanks. Just to clarify, by "unsupported" you mean unsupported in the
> current patch implementation right? From what I understand value
> unboxing could be done without parentheses in a non-top level select
> query.
>

Yes, it can be implemented in the next steps. I don't think there can be
some issues, but it means more lines and a little bit more complex
interface.
In this step, I try to implement minimalistic required functionality that
can be enhanced in next steps. For this area is an important fact, so
session variables
will be shadowed always by relations. It means new functionality in session
variables cannot break existing applications ever, and then there is space
for future enhancement.

>
> As a side note, I'm not sure if this branch is exercised in any tests.
> I've replaced returning InvalidOid with actually doing
> LookupVariable(NULL, a, true)
> in this case, and all the tests are still passing.
>

Usually we don't test not yet implemented functionality.

>
> > > Another question about how shadowing warning should work between
> > > namespaces.
> > > Let's say I've got two namespaces, public and test, both have a session
> > > variable with the same name, but only one has a table with the same
> name:
> > >
> > > -- in public
> > > create table test_agg(a int);
> > > create type for_test_agg as (a int);
> > > create variable test_agg for_test_agg;
> > >
> > > -- in test
> > > create type for_test_agg as (a int);
> > > create variable test_agg for_test_agg;
> > >
> > > Now if we will try to trigger the shadowing warning from public
> > > namespace, it would work differently:
> > >
> > > -- in public
> > > =# let test.test_agg.a = 10;
> > > =# let test_agg.a = 20;
> > > =# set session_variables_ambiguity_warning to on;
> > >
> > > -- note the value returned from the table
> > > =# select jsonb_agg(test_agg.a) from test_agg;
> > > WARNING: 42702: session variable "test_agg.a" is shadowed
> > > LINE 1: select jsonb_agg(test_agg.a) from test_agg;
> > > ^
> > > DETAIL: Session variables can be shadowed by columns,
> routine's
> > > variables and routine's arguments with the same name.
> > > LOCATION: transformColumnRef, parse_expr.c:940
> > > jsonb_agg
> > > -----------
> > > [1]
> > >
> > > -- no warning, note the session variable value
> > > =# select jsonb_agg(test.test_agg.a) from test_agg;
> > > jsonb_agg
> > > -----------
> > > [10]
> > >
> > > It happens because in the second scenario the logic inside
> > > transformColumnRef
> > > will not set up the node variable (there is no corresponding table in
> the
> > > "test" schema), and the following conditions covering session variables
> > > shadowing are depending on it. Is it supposed to be like this?
> > >
> >
> > I am sorry, I don't understand what you want to describe. Session
> variables
> > are shadowed by relations, ever. It is design. In the first case, the
> > variable is shadowed and a warning is raised. In the second case,
> > "test"."test_agg"."a" is a fully unique qualified identifier, and then
> the
> > variable is used, and then it is not shadowed.
>
> Yeah, there was a misunderstanding on my side, sorry. For whatever
> reason I thought shadowing between schemas is a reasonable thing, but as
> you pointed out it doesn't really make sense.
>

yes. Thinking about this question is not trivial. There are more dimensions
- like search path setting, catalog name, possible three fields identifier,
possible collisions between variable and variable, and between variable and
relation. But current design can work I think. Still it is strong enough,
and it is simplified against start design.

Regards

Pavel