Re: Schema variables - new implementation for Postgres 15

st 19. 1. 2022 v 9:01 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Tue, Jan 18, 2022 at 10:01:01PM +0100, Pavel Stehule wrote:
> > pá 14. 1. 2022 v 3:44 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com>
> napsal:
> >
> > >
> > > =# let myvariable = 'AA';
> > > LET
> > >
> > > =# select 'AA' collate "en-x-icu" < myvariable;
> > > ?column?
> > > ----------
> > > f
> > > (1 row)
> > >
> > > =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
> > > ERROR: 42P21: collation mismatch between explicit collations
> "en-x-icu"
> > > and "mycollation"
> > > LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
> > >
> >
> > What do you expect? I don't understand collating well, but it looks
> > correct. Minimally the tables have the same behavior.
>
> Indeed, I actually didn't know that such object's collation were implicit
> and
> could be overloaded without a problem as long as there's no conflict
> between
> all the explicit collations. So I agree that the current behavior is ok,
> including a correct handling for wanted conflicts:
>
> =# create variable var1 text collate "fr-x-icu";
> CREATE VARIABLE
>
> =# create variable var2 text collate "en-x-icu";
> CREATE VARIABLE
>
> =# let var1 = 'hoho';
> LET
>
> =# let var2 = 'hoho';
> LET
>
> =# select var1 < var2;
> ERROR: 42P22: could not determine which collation to use for string
> comparison
> HINT: Use the COLLATE clause to set the collation explicitly.
>
> > Please, can you check the attached patches?
>
> All the issue I mentioned are fixed, thanks!
>
>
thank you for check

>
> I see a few problems with the other new features added though. The new
> session_variables_ambiguity_warning GUC is called even in contexts where it
> shouldn't apply. For instance:
>
> =# set session_variables_ambiguity_warning = 1;
> SET
>
> =# create variable v text;
> CREATE VARIABLE
>
> =# DO $$
> DECLARE v text;
> BEGIN
> v := 'test';
> RAISE NOTICE 'v: %', v;
> END;
> $$ LANGUAGE plpgsql;
> WARNING: 42702: session variable "v" is shadowed by column
> LINE 1: v := 'test'
> ^
> DETAIL: The identifier can be column reference or session variable
> reference.
> HINT: The column reference is preferred against session variable
> reference.
> QUERY: v := 'test'
>
> But this "v := 'test'" shouldn't be a substitute for a LET, and it indeed
> doesn't work:
>

Yes, there are some mistakes (bugs). The PLpgSQL assignment as target
should not see session variables, so warning is nonsense there. RAISE
NOTICE should use local variables, and in this case is a question if we
should raise a warning. There are two possible analogies - we can see
session variables like global variables, and then the warning should not be
raised, or we can see relation between session variables and plpgsql
variables similar like session variables and some with higher priority, and
then warning should be raised. If we want to ensure that the new session
variable doesn't break code, then session variables should have lower
priority than plpgsql variables too. And because the plpgsql protection
against collision cannot be used, then I prefer raising the warning.

PLpgSQL assignment should not be in collision with session variables ever

>
> =# DO $$
> BEGIN
> v := 'test';
> RAISE NOTICE 'v: %', v;
> END;
> $$ LANGUAGE plpgsql;
> ERROR: 42601: "v" is not a known variable
> LINE 3: v := 'test';
>
> But the RAISE NOTICE does see the session variable (which should be the
> correct
> behavior I think), so the warning should have been raised for this
> instruction
> (and in that case the message is incorrect, as it's not shadowing a
> column).
>

In this case I can detect node type, and I can identify external param
node, but I cannot to detect if this code was executed from PLpgSQL or from
some other

So I can to modify warning text to some

DETAIL: The identifier can be column reference or query parameter or
session variable reference.
HINT: The column reference and query parameter is preferred against
session variable reference.

I cannot to use term "plpgsql variable" becase I cannot to ensure validity
of this message

Maybe is better to don't talk about source of this issue, and just talk
about result - so the warning text should be just

MESSAGE: "session variable \"xxxx\" is shadowed
DETAIL: "session variables can be shadowed by columns, routine's variables
and routine's arguments with same name"

Is it better?