Re: proposal: schema variables

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