Thanks for the fixes and the new patch set!
I think that this would be a very valuable feature!
This is a very incomplete review after playing with the patch set for a while.
Some bugs and oddities I have found:
"psql" support:
\? gives
\dV [PATTERN] list variables
I think that should say "schema variables" to distinguish them from
psql variables.
Running \dV when connected to an older server gives
ERROR: relation "pg_catalog.pg_variable" does not exist
LINE 16: FROM pg_catalog.pg_variable v
^
I think it would be better not to run the query and show a response like
session variables don't exist in server version 16
The LET statement:
CREATE VARIABLE testvar AS int4multirange[];
LET testvar = '{\{[2\,7]\,[11\,13]\}}';
ERROR: variable "laurenz.testvar" is of type int4multirange[], but
expression is of type text
LINE 1: LET testvar = '{\{[2\,7]\,[11\,13]\}}';
^
HINT: You will need to rewrite or cast the expression.
Sure, I can add an explicit type cast, but I think that the type should
be determined by the type of the variable. The right-hand side should be
treated as "unknown", and the type input function should be used.
Parameter session_variables_ambiguity_warning:
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -1544,6 +1544,16 @@ struct config_bool ConfigureNamesBool[] =
false,
NULL, NULL, NULL
},
+ {
+ {"session_variables_ambiguity_warning", PGC_USERSET,
DEVELOPER_OPTIONS,
+ gettext_noop("Raise warning when reference to a session
variable is ambiguous."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &session_variables_ambiguity_warning,
+ false,
+ NULL, NULL, NULL
+ },
I think the short_desc should be "Raise a warning" (with the indefinite
article).
DEVELOPER_OPTIONS is the wrong category. We normally use that for parameters
that are only relevant for PostgreSQL hackers. I think it should be
CLIENT_CONN_OTHER.
CREATE VARIABLE command:
CREATE VARIABLE str AS text NOT NULL;
ERROR: session variable must have a default value, since it's declared NOT
NULL
Perhaps this error message would be better:
session variables declared as NOT NULL must have a default value
This is buggy:
CREATE VARIABLE str AS text NOT NULL DEFAULT NULL;
Ugh.
SELECT str;
ERROR: null value is not allowed for NOT NULL session variable
"laurenz.str"
DETAIL: The result of DEFAULT expression is NULL.
Perhaps that is a leftover from the previous coding, but I think there need be
no check upon SELECT. It should be enough to check during CREATE VARIABLE and
LET.
pg_dump support:
The attempt to dump a database with an older version leads to
pg_dump: error: query failed: ERROR: relation "pg_catalog.pg_variable"
does not exist
LINE 14: FROM pg_catalog.pg_variable v
^
Dumping variables must be conditional on the server version.
IMMUTABLE variables:
+ <varlistentry id="sql-createvariable-immutable">
+ <term><literal>IMMUTABLE</literal></term>
+ <listitem>
+ <para>
+ The assigned value of the session variable can not be changed.
+ Only if the session variable doesn't have a default value, a single
+ initialization is allowed using the <command>LET</command> command.
Once
+ done, no further change is allowed until end of transaction
+ if the session variable was created with clause <literal>ON
TRANSACTION
+ END RESET</literal>, or until reset of all session variables by
+ <command>DISCARD VARIABLES</command>, or until reset of all session
+ objects by command <command>DISCARD ALL</command>.
+ </para>
+ </listitem>
+ </varlistentry>
I can see the usefulness of IMMUTABLE variables, but I am surprised that
they are reset by DISCARD. What is the use case you have in mind?
The use case I can envision is an application that sets a value right after
authentication, for use with row-level security. But then it would be harmful
if the user could reset the variable with DISCARD.
Documentation:
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
+ <para>
+ 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"/>. User can
explicitly
+ qualify the source object by syntax <literal>table.column</literal> or
+ <literal>variable.column</literal>.
+ </para>
I think you mean <literal>schema.variable</literal>, not
<literal>variable.column</literal>.
Yours,
Laurenz Albe
