Re: Schema variables - new implementation for Postgres 15

Hi

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!
>
>
> 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:
>
> =# DO $$
> BEGIN
> v := 'test';
> RAISE NOTICE 'v: %', v;
> END;
> $$ LANGUAGE plpgsql;
> ERROR: 42601: "v" is not a known variable
> LINE 3: v := 'test';
>

fixed

>
> 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).
>
> Also, the pg_dump handling emits a COLLATION option for session variables
> even
> for default collation, while it should only emit it if the collation is
> not the
> type's default collation. As a reference, for attributes the SQL used is:
>
> "CASE WHEN a.attcollation
> <> t.typcollation "
> "THEN a.attcollation ELSE
> 0 END AS attcollation,\n"
>

Isn't it a different issue? I don't see filtering DEFAULT_COLLATION_OID in
pg_dump code. But this case protects against a redundant COLLATE clause,
and for consistency, this check should be done for variables too.

<-->/*
<--> * Find all the user attributes and their types.
<--> *
<--> * Since we only want to dump COLLATE clauses for attributes whose
<--> * collation is different from their type's default, we use a CASE here
to
<--> * suppress uninteresting attcollations cheaply.
<--> */

fixed

>
> Also, should \dV or \dV+ show the collation?
>

I did it for \dV

>
> And a few comments on the new chunks in this version of the patch (I didn't
> look in detail at the whole patch yet):
>
> + <para>
> + The session variables can be overshadowed by columns in an query.
> When query
> + holds identifier or qualified identifier that can be used as session
> variable
> + identifier and as column identifier too, then it is used as column
> identifier
> + every time. This situation can be logged by enabling configuration
> + parameter <xref linkend="guc-session-variables-ambiguity-warning"/>.
> + </para>
>
> Is "overshadowed" correct? The rest of the patch only says "shadow(ed)".
>
> While at it, here's some proposition to improve the phrasing:
>
> + The session variables can be shadowed by column references in a query.
> When a
> + query contains identifiers or qualified identifiers that could be used
> as both
> + a session variable identifiers and as column identifier, then the column
> + identifier is preferred every time. Warnings can be emitted when this
> situation
> + happens by enabling configuration parameter <xref
> + linkend="guc-session-variables-ambiguity-warning"/>.
>
> Similarly, the next documentation could be rephrased to:
>
> + When on, a warning is raised when any identifier in a query could be
> used as both
> + a column identifier or a session variable identifier.
> + The default is <literal>off</literal>.
>
>
changed

>
> Few other nitpicking:
>
> + * If we really detect collision of column and variable
> identifier,
> + * then we prefer column, because we don't want to allow to
> break
> + * an existing valid queries by new variable.
>
> s/an existing/existing
>

refactorized

>
> +-- it is ambigonuous, but columns are preferred
>
> ambiguous?
>

fixed

>
>
> @@ -369,6 +367,19 @@ VariableCreate(const char *varName,
> /* dependency on extension */
> recordDependencyOnCurrentExtension(&myself, false);
>
> + /*
> + * Normal dependency from a domain to its collation. We know the
> default
> + * collation is pinned, so don't bother recording it.
> + */
> + if (OidIsValid(varCollation) &&
> + varCollation != DEFAULT_COLLATION_OID)
>
> The comment mentions domains rather than session variables.
>
>
fixed

> And for the initial patch, while looking around I found this comment on
> fix_alternative_subplan():
>

this is little bit strange - modified function is fix_scan_expr

>
> @@ -1866,7 +1969,9 @@ fix_alternative_subplan(PlannerInfo *root,
> AlternativeSubPlan *asplan,
> * replacing Aggref nodes that should be replaced by initplan output
> Params,
> * choosing the best implementation for AlternativeSubPlans,
> * looking up operator opcode info for OpExpr and related nodes,
> - * and adding OIDs from regclass Const nodes into
> root->glob->relationOids.
> + * and adding OIDs from regclass Const nodes into
> root->glob->relationOids,
> + * and replacing PARAM_VARIABLE paramid, that is the oid of the session
> variable
> + * to offset the array by query used session variables. ???
>
> I don't really understand the comment, and the "???" looks a bit
> suspicious.
> I'm assuming it's a reference to this new behavior in fix_param_node():
>

yes, I modified this comment

>
> * fix_param_node
> * Do set_plan_references processing on a Param
> + * Collect session variables list and replace variable oid by
> + * index to collected list.
> *
> * If it's a PARAM_MULTIEXPR, replace it with the appropriate Param from
> * root->multiexpr_params; otherwise no change is needed.
> * Just for paranoia's sake, we make a copy of the node in either case.
> + *
> + * If it's a PARAM_VARIABLE, then we should to calculate paramid.
>
> Some improvement on the comments would be welcome there, probably including
> some mention to the "glob->sessionVariables" collected list?
>

done

I am sending updated patches

Regards

Pavel