Re: Schema variables - new implementation for Postgres 15 - Mailing list pgsql-hackers

Also, the API doesn't look ideal.  AFAICS the only reason this function doesn't
error out in case of ambiguous name is that transformColumnRef may check if a
given name shadows a variable when session_variables_ambiguity_warning is set.
But since IdentifyVariable returns InvalidOid if the given list of identifiers
is ambiguous, it seems that the shadow detection can fail to detect a shadowed
reference if multiple variable would shadow the name:

# CREATE TYPE ab AS (a integer, b integer);
CREATE TYPE
# CREATE VARIABLE v_ab AS ab;
CREATE VARIABLE

# CREATE TABLE v_ab (a integer, b integer);
CREATE TABLE

# SET session_variables_ambiguity_warning = 1;
SET

# sELECT v_ab.a FROM v_ab;
WARNING:  42702: session variable "v_ab.a" is shadowed
LINE 1: select v_ab.a from v_ab;
               ^
DETAIL:  Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name.
 a
---
(0 rows)

# CREATE SCHEMA v_ab;
CREATE SCHEMA

# CREATE VARIABLE v_ab.a AS integer;
CREATE VARIABLE

# SELECT v_ab.a FROM v_ab;
 a
---
(0 rows)


Note that a bit later in transformColumnRef(), not_unique is checked only if
the returned varid is valid, which isn't correct as InvalidOid is currently
returned if not_unique is set.

I think that the error should be raised in IdentifyVariable rather than having
every caller check it.  I'm not sure how to perfectly handle the
session_variables_ambiguity_warning though.  Maybe make not_unique optional,
and error out if not_unique is null.  If not null, set it as necessary and
return one of the oid.  The only use would be for shadowing detection, and in
that case it won't be possible to check if a warning can be avoided as it would
be if no amgibuity is found, but that's probably ok.

Or maybe instead LookupVariable should have an extra argument to only match
variable with a composite type if caller asks to.  This would avoid scenarios
like:

CREATE VARIABLE myvar AS int;
SELECT myvar.blabla;
ERROR:  42809: type integer is not composite

Is that really ok to match a variable here rather than complaining about a
missing FROM-clause?

+       indirection_start = list_length(names) - (attrname ? 1 : 0);
+       indirection = list_copy_tail(stmt->target, indirection_start);
+ [...]
+               if (indirection != NULL)
+               {
+                       bool            targetIsArray;
+                       char       *targetName;
+
+                       targetName = get_session_variable_name(varid);
+                       targetIsArray = OidIsValid(get_element_type(typid));
+
+                       pstate->p_hasSessionVariables = true;
+
+                       coerced_expr = (Expr *)
+                               transformAssignmentIndirection(pstate,
+                                                                                          (Node *) param,
+                                                                                          targetName,
+                                                                                          targetIsArray,
+                                                                                          typid,
+                                                                                          typmod,
+                                                                                          InvalidOid,
+                                                                                          indirection,
+                                                                                          list_head(indirection),
+                                                                                          (Node *) expr,
+                                                                                          COERCION_PLPGSQL,
+                                                                                          stmt->location);
+               }

I'm not sure why you use this approach rather than just having something like
"ListCell *indirection_head", set it to a non-NULL value when needed, and use
that (with names) instead.  Note that it's also not correct to compare a List
to NULL, use NIL instead.

- expr_kind_allows_session_variables

Even if that's a bit annoying, I think it's better to explicitly put all values
there rather than having a default clause.

For instance, EXPR_KIND_CYCLE_MARK is currently allowing session variables,
which doesn't look ok.  It's probably just an error from when the patchset was
rebased, but this probably wouldn't happen if you get an error for an unmatched
value if you add a new expr kind (which doesn't happen that often).

[1] https://www.postgresql.org/message-id/CAFj8pRB2+pVBFsidS-AzhHdZid40OTUspWfXS0vgahHmaWosZQ@mail.gmail.com