Re: Schema variables - new implementation for Postgres 15

út 21. 3. 2023 v 17:18 odesílatel Peter Eisentraut <
peter(dot)eisentraut(at)enterprisedb(dot)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