Re: Schema variables - new implementation for Postgres 15

Hi

> 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.
>

done

I partially rewrote the IdentifyVariable routine. Now it should be robust.

>
> 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?
>

I feel "missing FROM-clause" is a little bit better, although the message
"type integer is not composite" is correct too. But there is agreement so
implementation of session variables should minimize impacts on PostgreSQL
behaviour, and it is more comfortant with some filtering used in other
places.

>
> + 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.
>

changed, fixed

>
> - 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).
>

done

updated patch assigned

Regards

Pavel

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