Re: Schema variables - new implementation for Postgres 15

út 21. 3. 2023 v 17:18 odesílatel Peter Eisentraut <
peter.eisentraut@enterprisedb.com> napsal:

> On 17.03.23 21:50, Pavel Stehule wrote:
> > rebase + fix-update pg_dump tests
>
> I have a few comments on the code:
>
> 0001
>
> ExecGrant_Variable() could probably use ExecGrant_common().
>

done


> The additions to syscache.c should be formatted to the new style.
>

done


>
> in pg_variable.h:
>



>
> - create_lsn ought to have a "var" prefix.
>

changed


>
> - typo: "typmode for variable's type"
>

fixed


>
> - What is the purpose of struct Variable?  It seems very similar to
>    FormData_pg_variable.  At least a comment would be useful.
>

I wrote comment there:


/*
 * The Variable struct is based on FormData_pg_variable struct. Against
 * FormData_pg_variable it can hold node of deserialized expression used
 * for calculation of default value.
 */

>
>
> Preserve the trailing comma in ParseExprKind.
>

done


>
>
> 0002
>
> expr_kind_allows_session_variables() should have some explanation
> about criteria for determining which expression kinds should allow
> variables.
>

I wrote comment there:

 /*
  * Returns true, when expression of kind allows using of
  * session variables.
+ *
+ * The session's variables can be used everywhere where
+ * can be used external parameters. Session variables
+ * are not allowed in DDL. Session's variables cannot be
+ * used in constraints.
+ *
+ * The identifier can be parsed as an session variable
+ * only in expression's kinds where session's variables
+ * are allowed. This is the primary usage of this function.
+ *
+ * Second usage of this function is for decision if
+ * an error message "column does not exist" or "column
+ * or variable does not exist" should be printed. When
+ * we are in expression, where session variables cannot
+ * be used, we raise the first form or error message.
  */


> Usually, we handle EXPR_KIND_* switches without default case, so we
> get notified what needs to be changed if a new enum symbol is added.
>

done


>
>
> 0010
>
> The material from the tutorial (advanced.sgml) might be better in
> ddl.sgml.
>

moved


>
> In catalogs.sgml, the columns don't match the ones actually defined in
> pg_variable.h in patch 0001 (e.g., create_lsn is missing and the order
> doesn't match).
>

fixed



>
> (The order of columns in pg_variable.h didn't immediately make sense to
> me either, so maybe there is a middle ground to be found.)
>

reordered. Still varcreate_lsn should be before varname column, because
sanity check:

--
-- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment
on
-- some of the C types that correspond to TYPALIGN_DOUBLE SQL types.  To
ensure
-- catalog C struct layout matches catalog tuple layout, arrange for the
tuple
-- offset of each fixed-width, attalign='d' catalog column to be divisible
by 8
-- unconditionally.  Keep such columns before the first NameData column of
the
-- catalog, since packagers can override NAMEDATALEN to an odd number.



>
> session_variables_ambiguity_warning: There needs to be more
> information about this.  The current explanation is basically just,
> "warn if your query is confusing".  Why do I want that?  Why would I
> not want that?  What is the alternative?  What are some examples?
> Shouldn't there be a standard behavior without a need to configure
> anything?
>

I enhanced this entry:

+       <para>
+        The session variables can be shadowed by column references in a
query. This
+        is an expected feature. The existing queries should not be broken
by creating
+        any session variable, because session variables are shadowed
always if the
+        identifier is ambiguous. The variables should be named without
possibility
+        to collision with identifiers of other database objects (column
names or
+        record field names). The warnings enabled by setting
<varname>session_variables_ambiguity_warning</varname>
+        should help with finding identifier's collisions.
+<programlisting>
+CREATE TABLE foo(a int);
+INSERT INTO foo VALUES(10);
+CREATE VARIABLE a int;
+LET a = 100;
+SELECT a FROM foo;
+</programlisting>
+
+<screen>
+ a
+----
+ 10
+(1 row)
+</screen>
+
+<programlisting>
+SET session_variables_ambiguity_warning TO on;
+SELECT a FROM foo;
+</programlisting>
+
+<screen>
+WARNING:  session variable "a" is shadowed
+LINE 1: SELECT a FROM foo;
+               ^
+DETAIL:  Session variables can be shadowed by columns, routine's variables
and routine's arguments with the same name.
+ a
+----
+ 10
+(1 row)
+</screen>
+       </para>
+       <para>
+        This feature can significantly increase size of logs, and then it
is
+        disabled by default, but for testing or development environments it
+        should be enabled.



>
> In allfiles.sgml, dropVariable should be before dropView.
>

fixed

Regards

Pavel