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