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
