Re: proposal: schema variables

pá 19. 7. 2024 v 13:41 odesílatel Laurenz Albe <laurenz.albe@cybertec.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
>