Re: proposal: schema variables

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
To: Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
Cc: Erik Rijkers <er(at)xs4all(dot)nl>, Michael Paquier <michael(at)paquier(dot)xyz>, Zhihong Yu <zyu(at)yugabyte(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, DUVAL REMI <REMI(dot)DUVAL(at)cheops(dot)fr>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: proposal: schema variables
Date: 2024-07-22 06:37:22
Message-ID: CAFj8pRBaD0_bMrCREWnVLfcTMdc0v7ns7Rt=sEvd1EoFmLfarQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-performance

pá 19. 7. 2024 v 13:41 odesílatel Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
napsal:

> 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
>

Attachment Content-Type Size
v20240722-0001-Enhancing-catalog-for-support-session-variables-and-.patch text/x-patch 131.7 KB
v20240722-0003-function-pg_session_variables-for-cleaning-tests.patch text/x-patch 4.6 KB
v20240722-0005-memory-cleaning-after-DROP-VARIABLE.patch text/x-patch 22.5 KB
v20240722-0004-DISCARD-VARIABLES.patch text/x-patch 9.6 KB
v20240722-0002-Storage-for-session-variables-and-SQL-interface.patch text/x-patch 146.7 KB
v20240722-0007-GUC-session_variables_ambiguity_warning.patch text/x-patch 13.6 KB
v20240722-0006-plpgsql-tests.patch text/x-patch 16.9 KB
v20240722-0009-PREPARE-LET-support.patch text/x-patch 7.4 KB
v20240722-0008-EXPLAIN-LET-support.patch text/x-patch 8.3 KB
v20240722-0010-implementation-of-temporary-session-variables.patch text/x-patch 38.7 KB
v20240722-0011-Implementation-ON-TRANSACTION-END-RESET-clause.patch text/x-patch 14.5 KB
v20240722-0012-Implementation-of-DEFAULT-clause-default-expressions.patch text/x-patch 33.4 KB
v20240722-0013-Implementation-of-NOT-NULL-and-IMMUTABLE-clauses.patch text/x-patch 36.0 KB
v20240722-0014-allow-read-an-value-of-session-variable-directly-fro.patch text/x-patch 12.0 KB
v20240722-0015-allow-parallel-execution-queries-with-session-variab.patch text/x-patch 11.3 KB
v20240722-0017-expression-with-session-variables-can-be-inlined.patch text/x-patch 4.2 KB
v20240722-0016-plpgsql-implementation-for-LET-statement.patch text/x-patch 13.6 KB
v20240722-0020-pg_restore-A-variable.patch text/x-patch 2.8 KB
v20240722-0018-this-patch-changes-error-message-column-doesn-t-exis.patch text/x-patch 29.1 KB
v20240722-0019-transactional-variables.patch text/x-patch 39.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Will Mortensen 2024-07-22 06:46:51 Re: Exposing the lock manager's WaitForLockers() to SQL
Previous Message kuroda.keisuke 2024-07-22 06:23:41 Re: Add privileges test for pg_stat_statements to improve coverage

Browse pgsql-performance by date

  From Date Subject
Next Message Laurenz Albe 2024-07-22 08:23:39 Re: proposal: schema variables
Previous Message Pavel Stehule 2024-07-20 19:48:43 Re: proposal: schema variables