Re: Schema variables - new implementation for Postgres 15

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@gmail.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';

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"

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

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


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

+-- it is ambigonuous, but columns are preferred

ambiguous?


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

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

@@ -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():

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