Re: proposal: schema variables - Mailing list pgsql-hackers

On Sat, 2021-04-10 at 08:58 +0200, Pavel Stehule wrote:
> I am sending a strongly updated patch for schema variables.
>
> I rewrote an execution of a LET statement. In the previous implementation I hacked
> STMT_SELECT. Now, I introduced a new statement STMT_LET, and I implemented a new
> executor node SetVariable. Now I think this implementation is much cleaner.
> Implementation with own executor node reduces necessary work on PL side - and allows
> the LET statement to be prepared - what is important from a security view.
>
> I'll try to write a second implementation based on a cleaner implementation like
> utility command too. I expect so this version will be more simple, but utility
> commands cannot be prepared, and probably, there should be special support for
> any PL. I hope a cleaner implementation can help to move this patch.
>
> We can choose one variant in the next step and this variant can be finalized.
>
> Notes, comments?

Thank you!

I tried to give the patch a spin, but it doesn't apply any more,
and there are too many conflicts for me to fix manually.

So I had a look at the documentation:

> --- a/doc/src/sgml/advanced.sgml
> +++ b/doc/src/sgml/advanced.sgml

> +   <para>
> +    The value of a schema variable is local to the current session. Retrieving
> +    a variable's value returns either a NULL or a default value, unless its value
> +    is set to something else in the current session with a LET command. The content
> +    of a variable is not transactional. This is the same as in regular variables
> +    in PL languages.
> +   </para>
> +
> +   <para>
> +    Schema variables are retrieved by the <command>SELECT</command> SQL command.
> +    Their value is set with the <command>LET</command> SQL command.
> +    While schema variables share properties with tables, their value cannot be updated
> +    with an <command>UPDATE</command> command.

"PL languages" -> "procedural languages".  Perhaps a link to the "procedural Languages"
chapter would be a good idea.
I don't think we should say "regular" variables: are there irregular variables?

My feeling is that "SQL statement <command>XY</command>" is better than
"<command>XY</command> SQL command".

I think the last sentence should go.  The properties they share with tables are
that they live in a schema and can be used with SELECT.
Also, it is not necessary to mention that they cannot be UPDATEd.  They cannot
be TRUNCATEd or CALLed either, so why mention UPDATE specifically?

> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml

> +     <row>
> +      <entry><structfield>varisnotnull</structfield></entry>
> +      <entry><type>boolean</type></entry>
> +      <entry></entry>
> +      <entry>
> +       True if the schema variable doesn't allow null value. The default value is false.
> +      </entry>
> +     </row>

I think the attribute should be called "varnotnull", similar to "attnotnull".
This attribute determines whether the variable is NOT NULL or not, not about
its current setting.

There is a plural missing: "doesn't allow null valueS".

> +     <row>
> +      <entry><structfield>vareoxaction</structfield></entry>
> +      <entry><type>char</type></entry>
> +      <entry></entry>
> +      <entry>
> +       <literal>n</literal> = no action, <literal>d</literal> = drop the variable,
> +       <literal>r</literal> = reset the variable to its default value.
> +      </entry>
> +     </row>

Perhaps the name "varxactendaction" would be better.

A descriptive sentence is missing.

> --- /dev/null
> +++ b/doc/src/sgml/ref/create_variable.sgml

> +  <para>
> +   The value of a schema variable is local to the current session. Retrieving
> +   a variable's value returns either a NULL or a default value, unless its value
> +   is set to something else in the current session with a LET command. The content
> +   of a variable is not transactional. This is the same as in regular variables in PL languages.
> +  </para>

"regular variables in PL languages" -> "variables in procedural languages"

> +  <para>
> +   Schema variables are retrieved by the <command>SELECT</command> SQL command.
> +   Their value is set with the <command>LET</command> SQL command.
> +   While schema variables share properties with tables, their value cannot be updated
> +   with an <command>UPDATE</command> command.
> +  </para>

That's just a literal copy from the tutorial section.  I have the same comments
as there.

> +   <varlistentry>
> +    <term><literal>NOT NULL</literal></term>
> +    <listitem>
> +     <para>
> +      The <literal>NOT NULL</literal> clause forbids to set the variable to
> +      a null value. A variable created as NOT NULL and without an explicitly
> +      declared default value cannot be read until it is initialized by a LET
> +      command. This obliges the user to explicitly initialize the variable
> +      content before reading it.
> +     </para>
> +    </listitem>
> +   </varlistentry>

What is the reason for that behavior?  I'd have expected that a NOT NULL
variable needs a default value.

> --- /dev/null
> +++ b/doc/src/sgml/ref/let.sgml

> +   <varlistentry>
> +    <term><literal>sql_expression</literal></term>
> +    <listitem>
> +     <para>
> +      An SQL expression. The result is cast into the schema variable's type.
> +     </para>
> +    </listitem>
> +   </varlistentry>

Always, even if there is no assignment or implicit cast?

I see no such wording fir INSERT or UPDATE, so if only assignment casts are used,
the second sentence should be removed.

> --- a/doc/src/sgml/ref/pg_restore.sgml
> +++ b/doc/src/sgml/ref/pg_restore.sgml

> +     <varlistentry>
> +      <term><option>-A <replaceable class="parameter">schema_variable</replaceable></option></term>
> +      <term><option>--variable=<replaceable class="parameter">schema_variable</replaceable></option></term>
> +      <listitem>
> +       <para>
> +        Restore a named schema variable only.  Multiple schema variables may be specified with
> +        multiple <option>-A</option> switches.
> +       </para>
> +      </listitem>
> +     </varlistentry>

Do we need that?  We have no such option for functions and other non-relations.

And if we really want such an option for "pg_restore", why not for "pg_dump"?

Yours,
Laurenz Albe