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".
probably, you are reading an old version of this patch. I cannot find these sentences.
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".
changed
> + <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.
I renamed field, recent version looks like
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>varxactendaction</structfield> <type>char</type>
</para>
<para>
Action performed at end of transaction:
<literal>n</literal> = no action, <literal>d</literal> = drop the variable,
<literal>r</literal> = reset the variable to its default value.
</para></entry>
</row>
> --- /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"
fixed
> + <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.
fixed
> + <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.
changed - now, the default is required when variable is NOT NULL
> --- /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?
It uses implicit cast in COERCION_ASSIGNMENT context. coerce_to_target_type is used always
This part of doc currently looks
<listitem>
<para>
An SQL expression (can be subquery in parenthesis). The result must
be of castable to the same data type as the session variable (in
implicit or assignment context).
</para>
</listitem>
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.
It is designed to be consistent with others. pg_restore supports functions -P, triggers -T
And if we really want such an option for "pg_restore", why not for "pg_dump"?
I have no strong opinion about it, I think so it is consistent with other non-relations, but it is not important.
I moved this feature to a separate patch. It can be committed optionaly or later.
pg_restore has options -P, -T, and pg_dump does not have these options. These options (functionality) can be implemented in pg_dump too, but unfortunately -T is used for different purposes (exclude table).
Regards
Pavel
Yours,
Laurenz Albe