Re: proposal: schema variables

[Pavel Stehule]

čt 1. 8. 2024 v 13:22 odesílatel Laurenz Albe 
napsal:

> On Thu, 2024-08-01 at 08:12 +0200, Pavel Stehule wrote:
> > fresh rebase + fix format in doc
>
> Thanks!
>
> I'm curious, but too lazy to build the patch now, so I'm asking:
> what did you do about this error?
>

I try to investigate this issue now.

The patchset is just merging of your work


> > CREATE VARIABLE var AS date;
> > LET var = current_date;
> > PREPARE stmt(date) AS SELECT $1;
> > EXECUTE stmt(var);
> > ERROR:  paramid of PARAM_VARIABLE param is out of range
>


>
> Or does a later patch take care of that?
>

This is a clear bug, and I have to fix it. I hope so I'll do this today



>
> Yours,
> Laurenz Albe
>

Re: proposal: schema variables

[Laurenz Albe]

On Thu, 2024-08-01 at 08:12 +0200, Pavel Stehule wrote:
> fresh rebase + fix format in doc

Thanks!

I'm curious, but too lazy to build the patch now, so I'm asking:
what did you do about this error?

> CREATE VARIABLE var AS date;
> LET var = current_date;
> PREPARE stmt(date) AS SELECT $1;
> EXECUTE stmt(var);
> ERROR:  paramid of PARAM_VARIABLE param is out of range

Or does a later patch take care of that?

Yours,
Laurenz Albe

Re: proposal: schema variables

[Pavel Stehule]

čt 1. 8. 2024 v 9:45 odesílatel Erik Rijkers  napsal:

> doc-build fails: a slash tumbled backwards in doc/src/sgml:
>
> $ grep 'xref.*.\\>'  *.sgml
> plpgsql.sgml: (see ).
>
> That should simply be a forward slash, of course.
>

should be fixed in my today patchset



>
> Thanks,
>
> Erik Rijkers
>
>
>
>
> Op 8/1/24 om 08:12 schreef Pavel Stehule:
> > Hi
> >
> > fresh rebase + fix format in doc
> >
> > Regards
> >
> > Pavel
> >
>

Re: proposal: schema variables

[Erik Rijkers]

doc-build fails: a slash tumbled backwards in doc/src/sgml:

$ grep 'xref.*.\\>'  *.sgml
plpgsql.sgml: (see ).

That should simply be a forward slash, of course.

Thanks,

Erik Rijkers




Op 8/1/24 om 08:12 schreef Pavel Stehule:

Hi

fresh rebase + fix format in doc

Regards

Pavel

Re: proposal: schema variables

[Pavel Stehule]

st 31. 7. 2024 v 8:57 odesílatel Laurenz Albe 
napsal:

> On Wed, 2024-07-31 at 08:41 +0200, Pavel Stehule wrote:
> > Probably you didn't attach new files - the second patch is not complete.
> Or you didn't make changes there?
>
> Hm.  What is missing?
>

let.sgml,
session_variable.c
svariable_receiver.c
session_variable.h
...

Regards

Pavel




>
> Yours,
> Laurenz Albe
>

Re: proposal: schema variables

[Laurenz Albe]

On Wed, 2024-07-31 at 08:41 +0200, Pavel Stehule wrote:
> Probably you didn't attach new files - the second patch is not complete. Or 
> you didn't make changes there?

Hm.  What is missing?

Yours,
Laurenz Albe

Re: proposal: schema variables

[Pavel Stehule]

út 30. 7. 2024 v 21:46 odesílatel Laurenz Albe 
napsal:

> This is my review of the second patch in the series.
>
> Again, I mostly changed code comments and documentation.
>
> Noteworthy remarks:
>
> - A general reminder: single line comments should start with a lower case
>   letter and have to period in the end:
>
>   /* this is a typical single line comment */
>
> - Variable as parameter:
>
>   CREATE VARIABLE var AS date;
>   LET var = current_date;
>   PREPARE stmt(date) AS SELECT $1;
>   EXECUTE stmt(var);
>   ERROR:  paramid of PARAM_VARIABLE param is out of range
>
>   Is that working as intended?  I don't understand the error message.
>
>   Perhaps there is a bug in src/backend/executor/execExpr.c.
>
> - IdentifyVariable
>
>   > --- a/src/backend/catalog/namespace.c
>   > +++ b/src/backend/catalog/namespace.c
>   > [...]
>   > +/*
>   > + * IdentifyVariable - try to find variable identified by list of
> names.
>   > [...]
>
>   The function comment doesn't make clear to me what the function does.
>   Perhaps that is so confusing because the function seems to serve several
>   purposes (during parsing, in ANALYZE, and to identify shadowed variables
>   in a later patch).
>
>   I rewrote the function comment, but didn't touch the code or code
> comments.
>
>   Perhaps part of the reason why this function is so complicated is that
>   you support things like this:
>
> CREATE SCHEMA sch;
> CREATE TYPE cp AS (a integer, b integer);
> CREATE VARIABLE sch.v AS cp;
> LET sch.v = (1, 2);
> SELECT sch.v.b;
>  b
> ═══
>  2
> (1 row)
>
> test=# SELECT test.sch.v.b;
>  b
> ═══
>  2
> (1 row)
>
>   We don't support that for tables:
>
> CREATE TABLE tab (c cp);
> INSERT INTO tab VALUES (ROW(42, 43));
> SELECT tab.c.b FROM tab;
> ERROR:  cross-database references are not implemented: tab.c.b
>
>   You have to use extra parentheses:
>
> SELECT (tab.c).b FROM tab;
>  b
> 
>  43
> (1 row)
>
>   I'd say that this should be the same for session variables.
>   What do you think?
>
>   Code comments:
>
>   - There are lots of variables declared at the beginning, but they are
> only
> used in code blocks further down.  For clarity, you should declare a
> variable in the code block where it is used.
>
>   - +   /*
> +* In this case, "a" is used as catalog name -
> check it.
> +*/
> +   if (strcmp(a, get_database_name(MyDatabaseId)) !=
> 0)
> +   {
> +   if (!noerror)
> +   ereport(ERROR,
> +
>  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +errmsg("cross-database references
> are not implemented: %s",
> +   NameListToString(names;
> +   }
> +   else
> +   {
>
> There needn't be an "else", since the ereport() will never return.
> Less indentation is better.
>
> - src/backend/commands/session_variable.c
>
>   There is one comment that confuses me and that I did not edit:
>
>   > +Datum
>   > +GetSessionVariable(Oid varid, bool *isNull, Oid *typid)
>   > +{
>   > +   SVariable   svar;
>   > +
>   > +   svar = get_session_variable(varid);
>   > +
>   > +   /*
>   > +* Although svar is freshly validated in this point, the
> svar->is_valid can
>   > +* be false, due possible accepting invalidation message inside
> domain
>   > +* check. Now, the validation is done after lock, that can also
> accept
>   > +* invalidation message, so validation should be trustful.
>   > +*
>   > +* For now, we don't need to repeat validation. Only svar should
> be valid
>   > +* pointer.
>   > +*/
>   > +   Assert(svar);
>   > +
>   > +   *typid = svar->typid;
>   > +
>   > +   return copy_session_variable_value(svar, isNull);
>   > +}
>
> - src/backend/executor/execMain.c
>
>   > @@ -196,6 +198,51 @@ standard_ExecutorStart(QueryDesc *queryDesc, int
> eflags)
>   > Assert(queryDesc->sourceText != NULL);
>   > estate->es_sourceText = queryDesc->sourceText;
>   >
>   > +   /*
>   > +* The executor doesn't work with session variables directly.
> Values of
>   > +* related session variables are copied to dedicated array, and
> this array
>   > +* is passed to executor.
>   > +*/
>
>   Why?  Is that a performance measure?  The comment should explain that.
>
> - parallel safety
>
>   > --- a/src/backend/optimizer/util/clauses.c
>   > +++ b/src/backend/optimizer/util/clauses.c
>   > @@ -935,6 +936,13 @@ max_parallel_hazard_walker(Node *node,
> max_parallel_hazard_context *context)
>   > if (param->paramkind == PARAM_EXTERN)
>   > return false;
>   >
>   > +   /* We doesn't support passing session variables to workers */
>   > +   if (param->paramkind == 

Re: proposal: schema variables

[Laurenz Albe]

Thanks for the updated patch set.

I found a problem in 0019-transactional-variables.patch:

--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -9851,6 +9851,17 @@ SCRAM-SHA-256$iteration 
count:
   
  
 
+ 
+  varistransact
+  boolean
+  
+  
+   True, when the variable is "transactional". In case of transaction
+   rollback, transactional variables are reset to their content at the
+   transaction start. The default value is false.
+  
+ 

That's messed up; it should be

 
  
   varistransact boolean
  
  
   True, when the variable is transactional. In the case
   of a transaction rollback, transactional variables are reset to the
   value they had when the transaction started. The default value is
   false.
  
 

I have started reading through the first patch, and so far I have only found
language problems.

I wonder how I should go about this.  At first, I intended to send an edited
version of the first patch, but as later patches depend on earlier patches,
that would mess up the patch set.

I can send my suggested modifications in text, but then you have to copy and
paste them all, which is cumbersome.

What would be best for you?


Thinking further, I wondered about the order of patches.
If some committer eventually takes mercy on this patch set, I expect that
only a part of the functionality will go in as a first step.
Does the order of the patches in the patch set match such a process?

I'd guess that temporary session variables or ON TRANSACTION END RESET
would be things that can be committed later on, but PL/pgSQL support
should be in the first commit.

What is your approach to that?

Yours,
Laurenz Albe

Re: proposal: schema variables

[Pavel Stehule]

út 23. 7. 2024 v 23:41 odesílatel Laurenz Albe 
napsal:

> On Tue, 2024-07-23 at 16:34 +0200, Laurenz Albe wrote:
> > CREATE VARIABLE command:
> >
> >   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.
>
> I'm having second thoughts about that.
>
> I was thinking of a variable like of a table column, but there is a
> fundamental
> difference: there is a clear moment when a tuple is added (INSERT or
> UPDATE),
> which is the point where a column can be checked for NULL values.
>
> A variable can be SELECTed without having been LET before, in which case it
> has the default value.  But there is no way to test the default value
> before
> the variable is SELECTed.  So while DEFAULT NULL for a non-nullable
> variable
> seems weird, it is no worse than DEFAULT somefunc() for a function that
> returns
> NULL.
>
> So perhaps the behavior I complained about above is actually the right one.
> In the view of that, it doesn't seem necessary to enforce a DEFAULT value
> for
> a NOT NULL variable: NOT NULL might just as well mean "you have to LET it
> before
> you can SELECT it".
>

exactly


>
> > IMMUTABLE variables:
> >
> > +   
> > +IMMUTABLE
> > +
> > + 
> > +  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 LET
> command. Once
> > +  done, no further change is allowed until end of transaction
> > +  if the session variable was created with clause ON
> TRANSACTION
> > +  END RESET, or until reset of all session variables
> by
> > +  DISCARD VARIABLES, or until reset of all
> session
> > +  objects by command DISCARD ALL.
> > + 
> > +
> > +   
> >
> >   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.
>
> I'm beginning to be uncertain about that as well.  You might want to use a
> connection pool, and you LET the variable when you take it out of the pool.
> When the session is returned to the pool, variables get DISCARDed.
>
> Sure, a user can call DISCARD, but only if he or she is in an interactive
> session.
>
> So perhaps it is good as it is.
>

I think this design should work. There are a lot of scenarios, where
session variables can be used well, and sure, there will be scenarios where
it doesn't work well, but now, I think it is a good balance between
usability, complexity and code complexity. There are a lot of lines, but
the code is almost very simple.

Regards

Pavel


>
> Yours,
> Laurenz Albe
>

Re: proposal: schema variables

[Laurenz Albe]

On Wed, 2024-07-24 at 17:19 +0200, Pavel Stehule wrote:
> >   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.
> 
> I think it is correct. When you use SELECT str, then DEFAULT expression is
> executed, and then the result is assigned to a variable, and there is NOT NULL
> guard, which raises an exception. The variable is not initialized when you run
> DDL, but it is initialized when you first read or write from/to the variable.
> The DEFAULT expression is not evaluated in DDL time. In this case, I can 
> detect
> the problem in DDL time because the result is transformed to NULL node, but
> generally there can be SQL non immutable function, and then I need to wait 
> until
> the DEFAULT expression will be evaluated - and it is time of first reading.
> Unfortunately, there is not an available check if some expression is NULL,
> that I can use in DDL time, but I have it in plpgsql_check.

That makes sense to me.

In that case, I think we can drop the requirement that NOT NULL variables
need a default clause.

> 
> >   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.
> 
> Primary I think about IMMUTABLE variables like about some form of cache.
> This cache is protected against unwanted repeated write - and can help with
> detection of this situation.

We can leave it as it is.  The IMMUTABLE feature need not go into the first
release, so that can be discussed some more later.

Thanks for the new patch set; I'll look at it soon.

Yours,
Laurenz Albe

Re: proposal: schema variables

[Laurenz Albe]

On Tue, 2024-07-23 at 16:34 +0200, Laurenz Albe wrote:
> CREATE VARIABLE command:
> 
>   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.

I'm having second thoughts about that.

I was thinking of a variable like of a table column, but there is a fundamental
difference: there is a clear moment when a tuple is added (INSERT or UPDATE),
which is the point where a column can be checked for NULL values.

A variable can be SELECTed without having been LET before, in which case it
has the default value.  But there is no way to test the default value before
the variable is SELECTed.  So while DEFAULT NULL for a non-nullable variable
seems weird, it is no worse than DEFAULT somefunc() for a function that returns
NULL.

So perhaps the behavior I complained about above is actually the right one.
In the view of that, it doesn't seem necessary to enforce a DEFAULT value for
a NOT NULL variable: NOT NULL might just as well mean "you have to LET it before
you can SELECT it".

> IMMUTABLE variables:
> 
> +   
> +IMMUTABLE
> +
> + 
> +  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 LET 
> command. Once
> +  done, no further change is allowed until end of transaction
> +  if the session variable was created with clause ON 
> TRANSACTION
> +  END RESET, or until reset of all session variables by
> +  DISCARD VARIABLES, or until reset of all session
> +  objects by command DISCARD ALL.
> + 
> +
> +   
> 
>   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.

I'm beginning to be uncertain about that as well.  You might want to use a
connection pool, and you LET the variable when you take it out of the pool.
When the session is returned to the pool, variables get DISCARDed.

Sure, a user can call DISCARD, but only if he or she is in an interactive 
session.

So perhaps it is good as it is.

Yours,
Laurenz Albe

Re: proposal: schema variables

[Laurenz Albe]

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
+   },
+   _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:

+   
+IMMUTABLE
+
+ 
+  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 LET command. 
Once
+  done, no further change is allowed until end of transaction
+  if the session variable was created with clause ON 
TRANSACTION
+  END RESET, or until reset of all session variables by
+  DISCARD VARIABLES, or until reset of all session
+  objects by command DISCARD ALL.
+ 
+
+   

  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

+   
+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 . User can 
explicitly
+qualify the source object by syntax table.column or
+variable.column.
+   

  I think you mean schema.variable, not 
variable.column.

Yours,
Laurenz Albe

Re: proposal: schema variables

[Pavel Stehule]

po 22. 7. 2024 v 10:23 odesílatel Laurenz Albe 
napsal:

> Thanks for the updated patch and the fixes!
>
> On Mon, 2024-07-22 at 08:37 +0200, Pavel Stehule wrote:
> > > > --- a/doc/src/sgml/ref/pg_restore.sgml
> > > > +++ b/doc/src/sgml/ref/pg_restore.sgml
> > >
> > > > + 
> > > > +  -A  class="parameter">schema_variable
> > > > +  --variable= class="parameter">schema_variable
> > > > +  
> > > > +   
> > > > +Restore a named schema variable only.  Multiple schema
> variables may be specified with
> > > > +multiple -A switches.
> > > > +   
> > > > +  
> > > > + 
> > >
> > > 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).
>
> Ah!  I didn't realize that -P and -T are the same.  So no objections,
> although I'm
> not sure if anyone will ever want to restore a single variable from a
> backup.
>

I wrote it mainly for completeness and symmetry. I can imagine only one use
case - possibility to offline trace the changes  of schema, but who knows.
The cost is just an occupation of 'A' char. Maybe it doesn't need a short
option, and a long option can be good enough.

Pavel



> Yours,
> Laurenz Albe
>

Re: proposal: schema variables

[Laurenz Albe]

Thanks for the updated patch and the fixes!

On Mon, 2024-07-22 at 08:37 +0200, Pavel Stehule wrote:
> > > --- a/doc/src/sgml/ref/pg_restore.sgml
> > > +++ b/doc/src/sgml/ref/pg_restore.sgml
> > 
> > > +     
> > > +      -A  > > class="parameter">schema_variable
> > > +      --variable= > > class="parameter">schema_variable
> > > +      
> > > +       
> > > +        Restore a named schema variable only.  Multiple schema variables 
> > > may be specified with
> > > +        multiple -A switches.
> > > +       
> > > +      
> > > +     
> > 
> > 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).

Ah!  I didn't realize that -P and -T are the same.  So no objections, although 
I'm
not sure if anyone will ever want to restore a single variable from a backup.

Yours,
Laurenz Albe

Re: proposal: schema variables

[Laurenz Albe]

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

> +   
> +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.
> +   
> +
> +   
> +Schema variables are retrieved by the SELECT SQL 
> command.
> +Their value is set with the LET SQL command.
> +While schema variables share properties with tables, their value cannot 
> be updated
> +with an UPDATE 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 XY" is better than
"XY 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

> + 
> +  varisnotnull
> +  boolean
> +  
> +  
> +   True if the schema variable doesn't allow null value. The default 
> value is false.
> +  
> + 

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

> + 
> +  vareoxaction
> +  char
> +  
> +  
> +   n = no action, d = drop the 
> variable,
> +   r = reset the variable to its default value.
> +  
> + 

Perhaps the name "varxactendaction" would be better.

A descriptive sentence is missing.

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

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

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

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

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

> +   
> +NOT NULL
> +
> + 
> +  The NOT NULL 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.
> + 
> +
> +   

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

> +   
> +sql_expression
> +
> + 
> +  An SQL expression. The result is cast into the schema variable's type.
> + 
> +
> +   

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

Re: proposal: schema variables

[Pavel Stehule]

Hi

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?

Regards

Pavel


schema-variables-rev2-20210410.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

so 13. 3. 2021 v 7:01 odesílatel Pavel Stehule 
napsal:

> Hi
>
> fresh rebase
>

only rebase

Regards

Pavel


>
> Pavel
>


schema-variables-20210404.patch.gz
Description: application/gzip

Re: proposal: schema variables - doc

[Pavel Stehule]

Hi



po 22. 3. 2021 v 10:47 odesílatel Pavel Stehule 
napsal:

> Hi
>
> st 17. 3. 2021 v 13:05 odesílatel Erik Rijkers  napsal:
>
>>
>> > On 2021.03.13. 07:01 Pavel Stehule  wrote:
>> > Hi
>> > fresh rebase
>> > [schema-variables-20210313.patch.gz]
>>
>>
>> Hi Pavel,
>>
>> I notice that the phrase 'schema variable' is not in the index at the end
>> ('bookindex.html').  Not good.
>>
>> It is also not in the index at the front of the manual - also not good.
>>
>> Maybe these two (front and back index) can be added?
>>
>
> I inserted new indexterm "schema variable", and now this part of
> bookindex.html looks like:
>
> schema variablealtering, ALTER VARIABLEchanging, LETdefining, CREATE
> VARIABLEdescription, Descriptionremoving, DROP VARIABLE
>
>
>
>>
>> If a user searches the pdf, the first occurrence he finds is at:
>>
>>   43.13.2.4. Global variables and constants
>>   (in itself that occurrence/mention is all right, but is should not be
>> the first find, I think)
>>
>> (I think there was in earlier versions of the patch an entry in the
>> 'contents', i.e., at the front of the manual).  I think it would be good to
>> have it in the front-index, pointing to either LET or CREATE VARIABLE, or
>> maybe even to a small introductory paragraph somewhere else (again, I seem
>> to remember that there was one in an earlier patch version).
>>
>
>
> I wrote new section to "advanced features" about schema variables
>
>
>>
>>
>> Of the new commands that this patch brings, 'LET' is the most immediately
>> illuminating for a user (even when a CREATE VARIABLE has to be done first.
>> There is an entry 'LET' in the index (good), but it would be better if that
>> with LET-entry too the phrase 'schema variable' occurred.  (I don't know if
>> that's possible)
>>
>>
>> Then, in the CREATE VARIABLE paragraphs it says
>>'Changing a schema variable is non-transactional by default.'
>>
>> I think that, unless there exists a mode where schema vars can be made
>> transactional, 'by default' should be deleted (and there is no such
>> 'transactional mode' for schema variables, is there?).  The 'Description'
>> also has such a 'By default' which is better removed for the same reason.
>>
>
> fixed
>
>
>>
>> In the CREATE VARIABLE page the example is:
>>
>> CREATE VARIABLE var1 AS integer;
>> SELECT var1;
>>
>> I suggest to make that
>>
>> CREATE VARIABLE var1 AS date;
>> LET var1 = (select current_date);
>> SELECT var1;
>>
>> So that the example immediately shows an application of functionality.
>>
>
> done
>
> Thank you for the documentation review.
>
> Updated patch attached
>
> Regards
>
> Pavel
>
>
fresh update with merged Eric's changes in documentation

Regards

Pavel


>
>>
>> Thanks,
>>
>> Erik Rijkers
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> >
>> > Pavel
>>
>


schema-variables-20210325.patch.gz
Description: application/gzip

Re: proposal: schema variables - doc

[Pavel Stehule]

Hi

st 17. 3. 2021 v 13:05 odesílatel Erik Rijkers  napsal:

>
> > On 2021.03.13. 07:01 Pavel Stehule  wrote:
> > Hi
> > fresh rebase
> > [schema-variables-20210313.patch.gz]
>
>
> Hi Pavel,
>
> I notice that the phrase 'schema variable' is not in the index at the end
> ('bookindex.html').  Not good.
>
> It is also not in the index at the front of the manual - also not good.
>
> Maybe these two (front and back index) can be added?
>

I inserted new indexterm "schema variable", and now this part of
bookindex.html looks like:

schema variablealtering, ALTER VARIABLEchanging, LETdefining, CREATE
VARIABLEdescription, Descriptionremoving, DROP VARIABLE



>
> If a user searches the pdf, the first occurrence he finds is at:
>
>   43.13.2.4. Global variables and constants
>   (in itself that occurrence/mention is all right, but is should not be
> the first find, I think)
>
> (I think there was in earlier versions of the patch an entry in the
> 'contents', i.e., at the front of the manual).  I think it would be good to
> have it in the front-index, pointing to either LET or CREATE VARIABLE, or
> maybe even to a small introductory paragraph somewhere else (again, I seem
> to remember that there was one in an earlier patch version).
>


I wrote new section to "advanced features" about schema variables


>
>
> Of the new commands that this patch brings, 'LET' is the most immediately
> illuminating for a user (even when a CREATE VARIABLE has to be done first.
> There is an entry 'LET' in the index (good), but it would be better if that
> with LET-entry too the phrase 'schema variable' occurred.  (I don't know if
> that's possible)
>
>
> Then, in the CREATE VARIABLE paragraphs it says
>'Changing a schema variable is non-transactional by default.'
>
> I think that, unless there exists a mode where schema vars can be made
> transactional, 'by default' should be deleted (and there is no such
> 'transactional mode' for schema variables, is there?).  The 'Description'
> also has such a 'By default' which is better removed for the same reason.
>

fixed


>
> In the CREATE VARIABLE page the example is:
>
> CREATE VARIABLE var1 AS integer;
> SELECT var1;
>
> I suggest to make that
>
> CREATE VARIABLE var1 AS date;
> LET var1 = (select current_date);
> SELECT var1;
>
> So that the example immediately shows an application of functionality.
>

done

Thank you for the documentation review.

Updated patch attached

Regards

Pavel



>
> Thanks,
>
> Erik Rijkers
>
>
>
>
>
>
>
>
>
>
>
>
>
> >
> > Pavel
>


schema-variables-20210322.patch.gz
Description: application/gzip

Re: proposal: schema variables - doc

[Erik Rijkers]

> On 2021.03.13. 07:01 Pavel Stehule  wrote:
> Hi
> fresh rebase
> [schema-variables-20210313.patch.gz]


Hi Pavel,

I notice that the phrase 'schema variable' is not in the index at the end 
('bookindex.html').  Not good.

It is also not in the index at the front of the manual - also not good.

Maybe these two (front and back index) can be added?


If a user searches the pdf, the first occurrence he finds is at:

  43.13.2.4. Global variables and constants
  (in itself that occurrence/mention is all right, but is should not be the 
first find, I think)

(I think there was in earlier versions of the patch an entry in the 'contents', 
i.e., at the front of the manual).  I think it would be good to have it in the 
front-index, pointing to either LET or CREATE VARIABLE, or maybe even to a 
small introductory paragraph somewhere else (again, I seem to remember that 
there was one in an earlier patch version).


Of the new commands that this patch brings, 'LET' is the most immediately 
illuminating for a user (even when a CREATE VARIABLE has to be done first.  
There is an entry 'LET' in the index (good), but it would be better if that 
with LET-entry too the phrase 'schema variable' occurred.  (I don't know if 
that's possible)


Then, in the CREATE VARIABLE paragraphs it says
   'Changing a schema variable is non-transactional by default.'

I think that, unless there exists a mode where schema vars can be made 
transactional, 'by default' should be deleted (and there is no such 
'transactional mode' for schema variables, is there?).  The 'Description' also 
has such a 'By default' which is better removed for the same reason.


In the CREATE VARIABLE page the example is:

CREATE VARIABLE var1 AS integer;
SELECT var1;

I suggest to make that

CREATE VARIABLE var1 AS date;
LET var1 = (select current_date);
SELECT var1;

So that the example immediately shows an application of functionality.


Thanks,

Erik Rijkers













> 
> Pavel

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh rebase

Pavel


schema-variables-20210313.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

út 16. 2. 2021 v 18:46 odesílatel Pavel Stehule 
napsal:

> Hi
>
> út 2. 2. 2021 v 9:43 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>> rebase and set PK for pg_variable table
>>
>
> rebase
>

rebase



> Pavel
>
>
>> Regards
>>
>> Pavel
>>
>


schema-variables-20200301.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

út 2. 2. 2021 v 9:43 odesílatel Pavel Stehule 
napsal:

> Hi
>
> rebase and set PK for pg_variable table
>

rebase

Pavel


> Regards
>
> Pavel
>


schema-variables-20200216.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

rebase and set PK for pg_variable table

Regards

Pavel


schema-variables-20210202.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

only rebase

Regards

Pavel


schema-variables-20200123.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

po 18. 1. 2021 v 15:24 odesílatel Erik Rijkers  napsal:

> On 2021-01-18 10:59, Pavel Stehule wrote:
> >>
> > and here is the patch
> > [schema-variables-20200118.patch.gz ]
>
>
> One small thing:
>
> The drop variable synopsis is:
>
> DROP VARIABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]
>
> In that text following it, 'RESTRICT' is not documented. When used it
> does not give an error but I don't see how it 'works'.
>

should be fixed now. Thank you for check

Regards

Pavel



>
> Erik
>
>
>
>


schema-variables-20200118-2.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Erik Rijkers]

On 2021-01-18 10:59, Pavel Stehule wrote:



and here is the patch
[schema-variables-20200118.patch.gz ]



One small thing:

The drop variable synopsis is:

DROP VARIABLE [ IF EXISTS ] name [, ...] [ CASCADE | RESTRICT ]

In that text following it, 'RESTRICT' is not documented. When used it 
does not give an error but I don't see how it 'works'.



Erik

Re: proposal: schema variables

[Pavel Stehule]

po 18. 1. 2021 v 10:50 odesílatel Pavel Stehule 
napsal:

>
>
> čt 14. 1. 2021 v 10:24 odesílatel Erik Rijkers  napsal:
>
>> On 2021-01-14 07:35, Pavel Stehule wrote:
>> > [schema-variables-20210114.patch.gz]
>>
>>
>> Build is fine. My (small) list of tests run OK.
>>
>> I did notice a few more documentation peculiarities:
>>
>>
>> 'The PostgreSQL has schema variables'  should be
>> 'PostgreSQL has schema variables'
>>
>>
> fixed
>
>
>> A link to the LET command should be added to the 'See Also' of the
>> CREATE VARIABLE, ALTER VARIABLE, and DROP VARIABLE pages. (After all,
>> the LET command is the most interesting)
>> Similarly, an ALTER VARIABLE link should be added to the 'See Also'
>> section of LET.
>>
>
> fixed
>
>
>>
>> Somehow, the sgml in the doc files causes too large spacing in the html,
>> example:
>> I copy from the LET html:
>> 'if that is defined.  If no explicit'
>> (6 spaces between 'defined.' and 'If')
>> Can you have a look?  Sorry - I can't find the cause.  It occurs on a
>> few more places in the newly added sgml/html.  The unwanted spaces are
>> visible also in the pdf.
>>
>
> Should be fixed in the last version. Probably there were some problems
> with invisible white chars.
>
> Thank you for check
>
> Pavel
>
>
>
>> (firefox 78.6.1, debian)
>>
>
and here is the patch

Regards

Pavel


>>
>> Thanks,
>>
>> Erik Rijkers
>>
>>
>>


schema-variables-20200118.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

čt 14. 1. 2021 v 11:31 odesílatel Josef Šimánek 
napsal:

> I did some testing locally. All runs smoothly, compiled without warning.
>
> Later on (once merged) it would be nice to write down a documentation
> page for the whole feature as a set next to documented individual
> commands.
> It took me a few moments to understand how this works.
>
> I was looking how to create variable with non default value in one
> command, but if I understand it correctly, that's not the purpose.
> Variable lives in a schema with default value, but you can set it per
> session via LET.
>
> Thus "CREATE VARIABLE" statement should not be usually part of
> "classic" queries, but it should be threatened more like TABLE or
> other DDL statements.
>
> On the other side LET is there to use the variable in "classic" queries.
>
> Does that make sense? Feel free to ping me if any help with
> documentation would be needed. I can try to prepare an initial
> variables guide once I'll ensure I do  understand this feature well.
>

I invite any help with doc. Maybe there can be page in section advanced
features

https://www.postgresql.org/docs/current/tutorial-advanced.html

Regards

Pavel


>
> PS: Now it is clear to me why it is called "schema variables", not
> "session variables".
>
> čt 14. 1. 2021 v 7:36 odesílatel Pavel Stehule 
> napsal:
> >
> > Hi
> >
> > rebase
> >
> > Regards
> >
> > Pavel
> >
>

Re: proposal: schema variables

[Pavel Stehule]

čt 14. 1. 2021 v 10:24 odesílatel Erik Rijkers  napsal:

> On 2021-01-14 07:35, Pavel Stehule wrote:
> > [schema-variables-20210114.patch.gz]
>
>
> Build is fine. My (small) list of tests run OK.
>
> I did notice a few more documentation peculiarities:
>
>
> 'The PostgreSQL has schema variables'  should be
> 'PostgreSQL has schema variables'
>
>
fixed


> A link to the LET command should be added to the 'See Also' of the
> CREATE VARIABLE, ALTER VARIABLE, and DROP VARIABLE pages. (After all,
> the LET command is the most interesting)
> Similarly, an ALTER VARIABLE link should be added to the 'See Also'
> section of LET.
>

fixed


>
> Somehow, the sgml in the doc files causes too large spacing in the html,
> example:
> I copy from the LET html:
> 'if that is defined.  If no explicit'
> (6 spaces between 'defined.' and 'If')
> Can you have a look?  Sorry - I can't find the cause.  It occurs on a
> few more places in the newly added sgml/html.  The unwanted spaces are
> visible also in the pdf.
>

Should be fixed in the last version. Probably there were some problems with
invisible white chars.

Thank you for check

Pavel



> (firefox 78.6.1, debian)
>
>
> Thanks,
>
> Erik Rijkers
>
>
>

Re: proposal: schema variables

[Josef Šimánek]

I did some testing locally. All runs smoothly, compiled without warning.

Later on (once merged) it would be nice to write down a documentation
page for the whole feature as a set next to documented individual
commands.
It took me a few moments to understand how this works.

I was looking how to create variable with non default value in one
command, but if I understand it correctly, that's not the purpose.
Variable lives in a schema with default value, but you can set it per
session via LET.

Thus "CREATE VARIABLE" statement should not be usually part of
"classic" queries, but it should be threatened more like TABLE or
other DDL statements.

On the other side LET is there to use the variable in "classic" queries.

Does that make sense? Feel free to ping me if any help with
documentation would be needed. I can try to prepare an initial
variables guide once I'll ensure I do  understand this feature well.

PS: Now it is clear to me why it is called "schema variables", not
"session variables".

čt 14. 1. 2021 v 7:36 odesílatel Pavel Stehule  napsal:
>
> Hi
>
> rebase
>
> Regards
>
> Pavel
>

Re: proposal: schema variables

[Erik Rijkers]

On 2021-01-14 07:35, Pavel Stehule wrote:

[schema-variables-20210114.patch.gz]



Build is fine. My (small) list of tests run OK.

I did notice a few more documentation peculiarities:


'The PostgreSQL has schema variables'  should be
'PostgreSQL has schema variables'


A link to the LET command should be added to the 'See Also' of the 
CREATE VARIABLE, ALTER VARIABLE, and DROP VARIABLE pages. (After all, 
the LET command is the most interesting)
Similarly, an ALTER VARIABLE link should be added to the 'See Also' 
section of LET.



Somehow, the sgml in the doc files causes too large spacing in the html, 
example:

I copy from the LET html:
   'if that is defined.  If no explicit'
   (6 spaces between 'defined.' and 'If')
Can you have a look?  Sorry - I can't find the cause.  It occurs on a 
few more places in the newly added sgml/html.  The unwanted spaces are 
visible also in the pdf.

(firefox 78.6.1, debian)


Thanks,

Erik Rijkers

Re: proposal: schema variables

[Pavel Stehule]

Hi

rebase

Regards

Pavel


schema-variables-20210114.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

pá 8. 1. 2021 v 18:54 odesílatel Erik Rijkers  napsal:

> On 2021-01-08 07:20, Pavel Stehule wrote:
> > Hi
> >
> > just rebase
> >
> > [schema-variables-20200108.patch]
>
> Hey Pavel,
>
> My gcc 8.3.0 compile says:
> (on debian 10/Buster)
>
> utility.c: In function ‘CreateCommandTag’:
> utility.c:2332:8: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
>  tag = CMDTAG_SELECT;
>  ^~~
> utility.c:2334:3: note: here
> case T_LetStmt:
> ^~~~
>

yes, there was an error from the last rebase. Fixed


>
> compile, check, check-world, runs without further problem.
>
> I also changed a few typos/improvements in the documentation, see
> attached.
>
> One thing I wasn't sure of: I have assumed that
>ON TRANSACTIONAL END RESET
>
> should be
>ON TRANSACTION END RESET
>
> and changed it accordingly, please double-check.
>

It looks well, I merged these changes to patch. Thank you very much for
these corectures

Updated patch attached

Regards

Pavel


>
> Erik Rijkers
>


schema-variables-20210110.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Erik Rijkers]

On 2021-01-08 07:20, Pavel Stehule wrote:

Hi

just rebase

[schema-variables-20200108.patch]


Hey Pavel,

My gcc 8.3.0 compile says:
(on debian 10/Buster)

utility.c: In function ‘CreateCommandTag’:
utility.c:2332:8: warning: this statement may fall through 
[-Wimplicit-fallthrough=]

tag = CMDTAG_SELECT;
^~~
utility.c:2334:3: note: here
   case T_LetStmt:
   ^~~~


compile, check, check-world, runs without further problem.

I also changed a few typos/improvements in the documentation, see 
attached.


One thing I wasn't sure of: I have assumed that
  ON TRANSACTIONAL END RESET

should be
  ON TRANSACTION END RESET

and changed it accordingly, please double-check.


Erik Rijkers
--- doc/src/sgml/ref/create_variable.sgml.orig	2021-01-08 17:40:20.181823036 +0100
+++ doc/src/sgml/ref/create_variable.sgml	2021-01-08 17:59:46.976127524 +0100
@@ -16,7 +16,7 @@
 
  
   CREATE VARIABLE
-  define a new permissioned typed schema variable
+  define a schema variable
  
 
  
@@ -29,24 +29,24 @@
   Description
 
   
-   The CREATE VARIABLE command creates a new schema variable.
+   The CREATE VARIABLE command creates a schema variable.
Schema variables, like relations, exist within a schema and their access is
controlled via GRANT and REVOKE commands.
-   Their changes are non-transactional by default.
+   Changing a schema variable is non-transactional by default.
   
 
   
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. By default,
-   the content of variable is not transactional, alike regular variables in PL languages.
+   the content of a variable is not transactional. This is the same as in regular variables in PL languages.
   
 
   
-   Schema variables are retrieved by the regular SELECT SQL command.
-   Their value can be set with the LET SQL command.
-   Notably, while schema variables share properties with tables, they cannot be updated
-   with UPDATE commands.
+   Schema variables are retrieved by the SELECT SQL command.
+   Their value is set with the LET SQL command.
+   While schema variables share properties with tables, their value cannot be updated
+   with an UPDATE command.
   
  
 
@@ -76,7 +76,7 @@
 name
 
  
-  The name (optionally schema-qualified) of the variable to create.
+  The name, optionally schema-qualified, of the variable.
  
 

@@ -85,7 +85,7 @@
 data_type
 
  
-  The name (optionally schema-qualified) of the data type of the variable to be created.
+  The name, optionally schema-qualified, of the data type of the variable.
  
 

@@ -105,7 +105,7 @@
 NOT NULL
 
  
-  The NOT NULL clause forbid to set the variable to
+  The NOT NULL 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
@@ -118,22 +118,22 @@
 DEFAULT default_expr
 
  
-  The DEFAULT clause assigns a default data to
-  schema variable.
+  The DEFAULT clause can be used to assign a default value to
+  a schema variable.
  
 

 

-ON COMMIT DROP, ON TRANSACTIONAL END RESET
+ON COMMIT DROP, ON TRANSACTION END RESET
 
  
   The ON COMMIT DROP clause specifies the behaviour
-  of temporary schema variable at transaction commit. With this clause the
+  of a temporary schema variable at transaction commit. With this clause the
       variable is dropped at commit time. The clause is only allowed
-      for temporary variables. The ON TRANSACTIONAL END RESET
+      for temporary variables. The ON TRANSACTION END RESET
   clause enforces the variable to be reset to its default value when
-  the transaction is either commited or rolled back.
+  the transaction is committed or rolled back.
  
 

@@ -145,7 +145,7 @@
   Notes
 
   
-   Use DROP VARIABLE command to remove a variable.
+   Use the DROP VARIABLE command to remove a variable.
   
  
 
--- doc/src/sgml/ref/discard.sgml.orig	2021-01-08 18:02:25.837531779 +0100
+++ doc/src/sgml/ref/discard.sgml	2021-01-08 18:40:09.973630164 +0100
@@ -104,6 +104,7 @@
 DISCARD PLANS;
 DISCARD TEMP;
 DISCARD SEQUENCES;
+DISCARD VARIABLES;
 
 

--- doc/src/sgml/ref/drop_variable.sgml.orig	2021-01-08 18:05:28.643147771 +0100
+++ doc/src/sgml/ref/drop_variable.sgml	2021-01-08 18:07:17.876113523 +0100
@@ -16,7 +16,7 @@
 
  
   DROP VARIABLE
-  removes a schema variable
+  remove a schema variable
  
 
  
@@ -52,7 +52,7 @@
 name
 
  
-  The name (optionally schema-qualified) of a schema variable.
+  The name, optionally schema-qualified, of a schema 

Re: proposal: schema variables

[Pavel Stehule]

Hi

just rebase

Regards

Pavel


schema-variables-20200108.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh rebase

Regards

Pavel


schema-variables-20210101.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

so 26. 12. 2020 v 7:18 odesílatel Erik Rijkers  napsal:

> On 2020-12-26 05:52, Pavel Stehule wrote:
> > so 19. 12. 2020 v 7:57 odesílatel Pavel Stehule
> > 
> > napsal:
> > [schema-variables-20201222.patch.gz (~]
> >
> >> Hi
> >>
> >> only rebase
> >>
> >
> > rebase and comments fixes
> >
>
> Hi Pavel,
>
> This file is the exact same as the file you sent Tuesday. Is it a
> mistake?
>

It is the same. Unfortunately,  it was sent in an isolated thread, and
commitfest applications didn't find the latest version. I tried to attach
new thread to the commitfest application, but without success.

Re: proposal: schema variables

[Erik Rijkers]

On 2020-12-26 05:52, Pavel Stehule wrote:
so 19. 12. 2020 v 7:57 odesílatel Pavel Stehule 


napsal:
[schema-variables-20201222.patch.gz (~]


Hi

only rebase



rebase and comments fixes



Hi Pavel,

This file is the exact same as the file you sent Tuesday. Is it a 
mistake?

Re: proposal: schema variables

[Pavel Stehule]

so 19. 12. 2020 v 7:57 odesílatel Pavel Stehule 
napsal:

> Hi
>
> only rebase
>

rebase and comments fixes

Regards

Pavel




> Regards
>
> Pavel
>


schema-variables-20201222.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

ne 20. 12. 2020 v 21:43 odesílatel Zhihong Yu  napsal:

> Hi,
> This is continuation of the previous review.
>
> +* We should to use schema variable buffer,
> when
> +* it is available.
>
> 'should use' (no to)
>

fixed


> +   /* When buffer of used schema variables loaded from shared memory
> */
>
> A verb seems missing in the above comment.
>

I changed this comment

<--><-->/*
<--><--> * link shared memory with working copy of schema variable's values
<--><--> * used in this query. This access is used by parallel query
executor's
<--><--> * workers.
<--><--> */


> +   elog(ERROR, "unexpected non-SELECT command in LET ... SELECT");
>
> Since non-SELECT is mentioned, I wonder if the trailing SELECT can be
> omitted.
>

done


> +* some collision can be solved simply here to reduce errors
> based
> +* on simply existence of some variables. Often error can be
> using
>
> simply occurred twice above - I think one should be enough.
> If you want to keep the second, it should be 'simple'.
>

I rewrote this comment

updated patch attached

Regards

Pavel



> Cheers
>
> On Sun, Dec 20, 2020 at 11:25 AM Zhihong Yu  wrote:
>
>> Hi,
>> I took a look at the rebased patch.
>>
>> +  varisnotnull
>> +  boolean
>> +  
>> +  
>> +   True if the schema variable doesn't allow null value. The default
>> value is false.
>>
>> I wonder whether the field can be named in positive tense: e.g.
>> varallowsnull with default of true.
>>
>> +  vareoxaction
>> +   n = no action, d = drop the
>> variable,
>> +   r = reset the variable to its default value.
>>
>> Looks like there is only one action allowed. I wonder if there is a
>> possibility of having two actions at the same time in the future.
>>
>> + The PL/pgSQL language has not packages
>> + and then it has not package variables and package constants. The
>>
>> 'has not' -> 'has no'
>>
>> +  a null value. A variable created as NOT NULL and without an
>> explicitely
>>
>> explicitely -> explicitly
>>
>> +   int nnewmembers;
>> +   Oid*oldmembers;
>> +   Oid*newmembers;
>>
>> I wonder if naming nnewmembers newmembercount would be more readable.
>>
>> For pg_variable_aclcheck:
>>
>> +   return ACLCHECK_OK;
>> +   else
>> +   return ACLCHECK_NO_PRIV;
>>
>> The 'else' can be omitted.
>>
>> + * Ownership check for a schema variables (specified by OID).
>>
>> 'a schema variable' (no s)
>>
>> For NamesFromList():
>>
>> +   if (IsA(n, String))
>> +   {
>> +   result = lappend(result, n);
>> +   }
>> +   else
>> +   break;
>>
>> There would be no more name if current n is not a String ?
>>
>> +* both variants, and returns InvalidOid with not_uniq flag,
>> when
>>
>> 'and return' (no s)
>>
>> +   return InvalidOid;
>> +   }
>> +   else if (OidIsValid(varoid_without_attr))
>>
>> 'else' is not needed (since the if block ends with return).
>>
>> For clean_cache_callback(),
>>
>> +   if (hash_search(schemavarhashtab,
>> +   (void *) >varid,
>> +   HASH_REMOVE,
>> +   NULL) == NULL)
>> +   elog(DEBUG1, "hash table corrupted");
>>
>> Maybe add more information to the debug, such as var name.
>> Should we come out of the while loop in this scenario ?
>>
>> Cheers
>>
>


schema-variables-20201222.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

ne 20. 12. 2020 v 20:24 odesílatel Zhihong Yu  napsal:

> Hi,
> I took a look at the rebased patch.
>
> +  varisnotnull
> +  boolean
> +  
> +  
> +   True if the schema variable doesn't allow null value. The default
> value is false.
>
> I wonder whether the field can be named in positive tense: e.g.
> varallowsnull with default of true.
>

although I prefer positive designed variables, in this case this negative
form is better due consistency with other system tables

postgres=# select table_name, column_name from information_schema.columns
where column_name like '%null';
┌──┬──┐
│  table_name  │ column_name  │
╞══╪══╡
│ pg_type  │ typnotnull   │
│ pg_attribute │ attnotnull   │
│ pg_variable  │ varisnotnull │
└──┴──┘
(3 rows)



> +  vareoxaction
> +   n = no action, d = drop the
> variable,
> +   r = reset the variable to its default value.
>

> Looks like there is only one action allowed. I wonder if there is a
> possibility of having two actions at the same time in the future.
>


Surely not - these three possibilities are supported and tested

CREATE VARIABLE t1 AS int DEFAULT -1 ON TRANSACTION END RESET
CREATE TEMP VARIABLE g AS int ON COMMIT DROP;


>
> + The PL/pgSQL language has not packages
> + and then it has not package variables and package constants. The
>
> 'has not' -> 'has no'
>

fixed


> +  a null value. A variable created as NOT NULL and without an
> explicitely
>
> explicitely -> explicitly
>

fixed


> +   int nnewmembers;
> +   Oid*oldmembers;
> +   Oid*newmembers;
>
> I wonder if naming nnewmembers newmembercount would be more readable.
>

It is not bad idea, but nnewmembers is used more times on more places, and
then this refactoring should be done in independent patch


> For pg_variable_aclcheck:
>
> +   return ACLCHECK_OK;
> +   else
> +   return ACLCHECK_NO_PRIV;
>
> The 'else' can be omitted.
>

again - this pattern is used more often in related source file, and I used
it for consistency with other functions. It is not visible from the patch,
but when you check the related file, it will be clean.


> + * Ownership check for a schema variables (specified by OID).
>
> 'a schema variable' (no s)
>
> For NamesFromList():
>
> +   if (IsA(n, String))
> +   {
> +   result = lappend(result, n);
> +   }
> +   else
> +   break;
>
> There would be no more name if current n is not a String ?
>

It tries to derive the name of a variable from the target list. Usually
there  can be only strings, but there can be array subscripting too
(A_indices node)
I wrote a comment there.


>
> +* both variants, and returns InvalidOid with not_uniq flag,
> when
>
> 'and return' (no s)
>
> +   return InvalidOid;
> +   }
> +   else if (OidIsValid(varoid_without_attr))
>
> 'else' is not needed (since the if block ends with return).
>

I understand. The `else` is not necessary, but I think so it is better due
symmetry

if ()
{
  return ..
}
else if ()
{
  return ..
}
else
{
  return ..
}

This style is used more times in Postgres, and usually I prefer it, when I
want to emphasize so all ways have some similar pattern. My opinion about
it is not too strong, The functionality is same, and I think so readability
is a little bit better (due symmetry) (but I understand well so this can be
subjective).



> For clean_cache_callback(),
>
> +   if (hash_search(schemavarhashtab,
> +   (void *) >varid,
> +   HASH_REMOVE,
> +   NULL) == NULL)
> +   elog(DEBUG1, "hash table corrupted");
>
> Maybe add more information to the debug, such as var name.
> Should we come out of the while loop in this scenario ?
>

I checked other places, and the same pattern is used in this text. If there
are problems, then the problem is not with some specific schema variable,
but in implementation of a hash table.

Maybe it is better to ignore the result (I did it), like it is done on some
other places. This part is implementation of some simple garbage collector,
and is not important if value was removed this or different way. I changed
comments for this routine.

Regards

Pavel


> Cheers
>

Re: proposal: schema variables

[Zhihong Yu]

Hi,
This is continuation of the previous review.

+* We should to use schema variable buffer, when
+* it is available.

'should use' (no to)

+   /* When buffer of used schema variables loaded from shared memory */

A verb seems missing in the above comment.

+   elog(ERROR, "unexpected non-SELECT command in LET ... SELECT");

Since non-SELECT is mentioned, I wonder if the trailing SELECT can be
omitted.

+* some collision can be solved simply here to reduce errors
based
+* on simply existence of some variables. Often error can be
using

simply occurred twice above - I think one should be enough.
If you want to keep the second, it should be 'simple'.

Cheers

On Sun, Dec 20, 2020 at 11:25 AM Zhihong Yu  wrote:

> Hi,
> I took a look at the rebased patch.
>
> +  varisnotnull
> +  boolean
> +  
> +  
> +   True if the schema variable doesn't allow null value. The default
> value is false.
>
> I wonder whether the field can be named in positive tense: e.g.
> varallowsnull with default of true.
>
> +  vareoxaction
> +   n = no action, d = drop the
> variable,
> +   r = reset the variable to its default value.
>
> Looks like there is only one action allowed. I wonder if there is a
> possibility of having two actions at the same time in the future.
>
> + The PL/pgSQL language has not packages
> + and then it has not package variables and package constants. The
>
> 'has not' -> 'has no'
>
> +  a null value. A variable created as NOT NULL and without an
> explicitely
>
> explicitely -> explicitly
>
> +   int nnewmembers;
> +   Oid*oldmembers;
> +   Oid*newmembers;
>
> I wonder if naming nnewmembers newmembercount would be more readable.
>
> For pg_variable_aclcheck:
>
> +   return ACLCHECK_OK;
> +   else
> +   return ACLCHECK_NO_PRIV;
>
> The 'else' can be omitted.
>
> + * Ownership check for a schema variables (specified by OID).
>
> 'a schema variable' (no s)
>
> For NamesFromList():
>
> +   if (IsA(n, String))
> +   {
> +   result = lappend(result, n);
> +   }
> +   else
> +   break;
>
> There would be no more name if current n is not a String ?
>
> +* both variants, and returns InvalidOid with not_uniq flag,
> when
>
> 'and return' (no s)
>
> +   return InvalidOid;
> +   }
> +   else if (OidIsValid(varoid_without_attr))
>
> 'else' is not needed (since the if block ends with return).
>
> For clean_cache_callback(),
>
> +   if (hash_search(schemavarhashtab,
> +   (void *) >varid,
> +   HASH_REMOVE,
> +   NULL) == NULL)
> +   elog(DEBUG1, "hash table corrupted");
>
> Maybe add more information to the debug, such as var name.
> Should we come out of the while loop in this scenario ?
>
> Cheers
>

Re: proposal: schema variables

[Zhihong Yu]

Hi,
I took a look at the rebased patch.

+  varisnotnull
+  boolean
+  
+  
+   True if the schema variable doesn't allow null value. The default
value is false.

I wonder whether the field can be named in positive tense: e.g.
varallowsnull with default of true.

+  vareoxaction
+   n = no action, d = drop the
variable,
+   r = reset the variable to its default value.

Looks like there is only one action allowed. I wonder if there is a
possibility of having two actions at the same time in the future.

+ The PL/pgSQL language has not packages
+ and then it has not package variables and package constants. The

'has not' -> 'has no'

+  a null value. A variable created as NOT NULL and without an
explicitely

explicitely -> explicitly

+   int nnewmembers;
+   Oid*oldmembers;
+   Oid*newmembers;

I wonder if naming nnewmembers newmembercount would be more readable.

For pg_variable_aclcheck:

+   return ACLCHECK_OK;
+   else
+   return ACLCHECK_NO_PRIV;

The 'else' can be omitted.

+ * Ownership check for a schema variables (specified by OID).

'a schema variable' (no s)

For NamesFromList():

+   if (IsA(n, String))
+   {
+   result = lappend(result, n);
+   }
+   else
+   break;

There would be no more name if current n is not a String ?

+* both variants, and returns InvalidOid with not_uniq flag,
when

'and return' (no s)

+   return InvalidOid;
+   }
+   else if (OidIsValid(varoid_without_attr))

'else' is not needed (since the if block ends with return).

For clean_cache_callback(),

+   if (hash_search(schemavarhashtab,
+   (void *) >varid,
+   HASH_REMOVE,
+   NULL) == NULL)
+   elog(DEBUG1, "hash table corrupted");

Maybe add more information to the debug, such as var name.
Should we come out of the while loop in this scenario ?

Cheers

Re: proposal: schema variables

[Pavel Stehule]

Hi

only rebase

Regards

Pavel


schema-variables-20201219.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 1. 10. 2020 v 7:08 odesílatel Pavel Stehule 
napsal:

>
>
> čt 1. 10. 2020 v 5:38 odesílatel Michael Paquier 
> napsal:
>
>> On Thu, Sep 24, 2020 at 08:49:50PM +0200, Pavel Stehule wrote:
>> > fixed patch attached
>>
>> It looks like there are again conflicts within setrefs.c.
>>
>
> fresh patch
>

rebase



> Regards
>
> Pavel
>
> --
>> Michael
>>
>


schema-variables-20201110.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 1. 10. 2020 v 5:38 odesílatel Michael Paquier 
napsal:

> On Thu, Sep 24, 2020 at 08:49:50PM +0200, Pavel Stehule wrote:
> > fixed patch attached
>
> It looks like there are again conflicts within setrefs.c.
>

fresh patch

Regards

Pavel

--
> Michael
>


schema-variables-20201001.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Michael Paquier]

On Thu, Sep 24, 2020 at 08:49:50PM +0200, Pavel Stehule wrote:
> fixed patch attached

It looks like there are again conflicts within setrefs.c.
--
Michael


signature.asc
Description: PGP signature

Re: proposal: schema variables

[Pavel Stehule]

čt 24. 9. 2020 v 5:58 odesílatel Pavel Stehule 
napsal:

>
>
> čt 24. 9. 2020 v 5:56 odesílatel Michael Paquier 
> napsal:
>
>> On Sat, Jul 11, 2020 at 06:44:24AM +0200, Pavel Stehule wrote:
>> > rebase
>>
>> Per the CF bot, this needs an extra rebase as it does not apply
>> anymore.  This has not attracted much the attention of committers as
>> well.
>>
>
> I'll fix it today
>

fixed patch attached

Regards

Pavel


> --
>> Michael
>>
>


schema-variables-20200924.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 24. 9. 2020 v 5:56 odesílatel Michael Paquier 
napsal:

> On Sat, Jul 11, 2020 at 06:44:24AM +0200, Pavel Stehule wrote:
> > rebase
>
> Per the CF bot, this needs an extra rebase as it does not apply
> anymore.  This has not attracted much the attention of committers as
> well.
>

I'll fix it today

--
> Michael
>

Re: proposal: schema variables

[Michael Paquier]

On Sat, Jul 11, 2020 at 06:44:24AM +0200, Pavel Stehule wrote:
> rebase

Per the CF bot, this needs an extra rebase as it does not apply
anymore.  This has not attracted much the attention of committers as
well.
--
Michael


signature.asc
Description: PGP signature

Re: proposal: schema variables

[Pavel Stehule]

po 6. 7. 2020 v 10:17 odesílatel Pavel Stehule 
napsal:

>
>
> ne 5. 7. 2020 v 15:33 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> čt 21. 5. 2020 v 14:49 odesílatel Pavel Stehule 
>> napsal:
>>
>>>
>>>
>>> čt 21. 5. 2020 v 13:34 odesílatel Amit Kapila 
>>> napsal:
>>>
 On Thu, May 21, 2020 at 3:41 PM Pavel Stehule 
 wrote:
 >
 > Hi
 >
 > just rebase without any other changes
 >

 You seem to forget attaching the rebased patch.

>>>
>>> I am sorry
>>>
>>> attached.
>>>
>>
>> fresh rebase
>>
>
> fix test build
>

rebase

Pavel


> Regards
>
> Pavel
>
>
>> Regards
>>
>> Pavel
>>
>>
>>> Pavel
>>>
>>>
 --
 With Regards,
 Amit Kapila.
 EnterpriseDB: http://www.enterprisedb.com

>>>


schema-variables-20200711.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

ne 5. 7. 2020 v 15:33 odesílatel Pavel Stehule 
napsal:

>
>
> čt 21. 5. 2020 v 14:49 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> čt 21. 5. 2020 v 13:34 odesílatel Amit Kapila 
>> napsal:
>>
>>> On Thu, May 21, 2020 at 3:41 PM Pavel Stehule 
>>> wrote:
>>> >
>>> > Hi
>>> >
>>> > just rebase without any other changes
>>> >
>>>
>>> You seem to forget attaching the rebased patch.
>>>
>>
>> I am sorry
>>
>> attached.
>>
>
> fresh rebase
>

fix test build

Regards

Pavel


> Regards
>
> Pavel
>
>
>> Pavel
>>
>>
>>> --
>>> With Regards,
>>> Amit Kapila.
>>> EnterpriseDB: http://www.enterprisedb.com
>>>
>>


schema-variables-20200706.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 21. 5. 2020 v 14:49 odesílatel Pavel Stehule 
napsal:

>
>
> čt 21. 5. 2020 v 13:34 odesílatel Amit Kapila 
> napsal:
>
>> On Thu, May 21, 2020 at 3:41 PM Pavel Stehule 
>> wrote:
>> >
>> > Hi
>> >
>> > just rebase without any other changes
>> >
>>
>> You seem to forget attaching the rebased patch.
>>
>
> I am sorry
>
> attached.
>

fresh rebase

Regards

Pavel


> Pavel
>
>
>> --
>> With Regards,
>> Amit Kapila.
>> EnterpriseDB: http://www.enterprisedb.com
>>
>


schema-variables-20200705.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 21. 5. 2020 v 13:34 odesílatel Amit Kapila 
napsal:

> On Thu, May 21, 2020 at 3:41 PM Pavel Stehule 
> wrote:
> >
> > Hi
> >
> > just rebase without any other changes
> >
>
> You seem to forget attaching the rebased patch.
>

I am sorry

attached.

Pavel


> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>


schema-variables-20200521.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Amit Kapila]

On Thu, May 21, 2020 at 3:41 PM Pavel Stehule  wrote:
>
> Hi
>
> just rebase without any other changes
>

You seem to forget attaching the rebased patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: proposal: schema variables

[Pavel Stehule]

Hi

just rebase without any other changes

Regards

Pavel

Re: proposal: schema variables

[Pavel Stehule]

Hi

rebase

Regards

Pavel

ne 22. 3. 2020 v 8:40 odesílatel Pavel Stehule 
napsal:

> Hi
>
> rebase
>
> Regards
>
> Pavel
>


schema-variables-20200310.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

rebase

Regards

Pavel


schema-variables-20200322.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

pá 20. 3. 2020 v 8:18 odesílatel Pavel Stehule 
napsal:

>
>
> čt 19. 3. 2020 v 10:43 odesílatel DUVAL REMI 
> napsal:
>
>> Hello
>>
>>
>>
>> I played around with the ALTER VARIABLE statement to make sure it’s OK
>> and it seems fine to me.
>>
>>
>>
>> Another last thing before commiting.
>>
>>
>>
>> I agree with Thomas Vondra, that this part might/should be simplified :
>>
>>
>>
>> [ { ON COMMIT DROP | ON { TRANSACTIONAL | TRANSACTION } END RESET } ]
>>
>>
>>
>> I would only allow “ON TRANSACTION END RESET”.
>>
>> I think we don’t need both here.
>>
>> Philippe Beaudoin was indeed talking about the TRANSACTIONAL keyword, but
>> that would have make sense (and I think that’s what he meant) , if you
>> could do something like “CREATE [NON] TRANSACTIONAL VARIABLE …”.
>>
>> But here I don’t think that the ON TRANSACTIONAL END RESET has any sense
>> in English, and it only complicates the syntax.
>>
>>
>>
>> Maybe Thomas Vondra (if it’s him) would be more inclined to commit the
>> patch if it has this more simple syntax has he requested.
>>
>>
>>
>> What do you think ?
>>
>
> I removed TRANSACTIONAL from this clause, see attachement change.diff
>
> Updated patch attached.
>
> I hope it would be the last touch, making it fully ready for a commiter.
>>
>
> Thank you very much for review and testing
>

documentation fix



> Pavel
>
>>


schema-variables-20200320-2.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 19. 3. 2020 v 10:43 odesílatel DUVAL REMI  napsal:

> Hello
>
>
>
> I played around with the ALTER VARIABLE statement to make sure it’s OK and
> it seems fine to me.
>
>
>
> Another last thing before commiting.
>
>
>
> I agree with Thomas Vondra, that this part might/should be simplified :
>
>
>
> [ { ON COMMIT DROP | ON { TRANSACTIONAL | TRANSACTION } END RESET } ]
>
>
>
> I would only allow “ON TRANSACTION END RESET”.
>
> I think we don’t need both here.
>
> Philippe Beaudoin was indeed talking about the TRANSACTIONAL keyword, but
> that would have make sense (and I think that’s what he meant) , if you
> could do something like “CREATE [NON] TRANSACTIONAL VARIABLE …”.
>
> But here I don’t think that the ON TRANSACTIONAL END RESET has any sense
> in English, and it only complicates the syntax.
>
>
>
> Maybe Thomas Vondra (if it’s him) would be more inclined to commit the
> patch if it has this more simple syntax has he requested.
>
>
>
> What do you think ?
>

I removed TRANSACTIONAL from this clause, see attachement change.diff

Updated patch attached.

I hope it would be the last touch, making it fully ready for a commiter.
>

Thank you very much for review and testing

Pavel

>
diff --git a/doc/src/sgml/ref/create_variable.sgml b/doc/src/sgml/ref/create_variable.sgml
index 1b1883a11a..cf175e05c6 100644
--- a/doc/src/sgml/ref/create_variable.sgml
+++ b/doc/src/sgml/ref/create_variable.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  
 
 CREATE [ { TEMPORARY | TEMP } ] [ IMMUTABLE ] VARIABLE [ IF NOT EXISTS ] name [ AS ] data_type ] [ COLLATE collation ]
-[ NOT NULL ] [ DEFAULT default_expr ] [ { ON COMMIT DROP | ON { TRANSACTIONAL | TRANSACTION } END RESET } ]
+[ NOT NULL ] [ DEFAULT default_expr ] [ { ON COMMIT DROP | ON { TRANSACTION } END RESET } ]
 
  
  
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 744342733d..c135a35d07 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4567,7 +4567,6 @@ OptSchemaVarDefExpr: DEFAULT b_expr	{ $$ = $2; }
 
 OnEOXActionOption:  ON COMMIT DROP	{ $$ = VARIABLE_EOX_DROP; }
 			| ON TRANSACTION END_P RESET			{ $$ = VARIABLE_EOX_RESET; }
-			| ON TRANSACTIONAL END_P RESET			{ $$ = VARIABLE_EOX_RESET; }
 			| /*EMPTY*/{ $$ = VARIABLE_EOX_NOOP; }
 		;
 
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index d39bcfe9cf..d26b0efcd9 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5978,7 +5978,7 @@
   proname => 'pg_collation_is_visible', procost => '10', provolatile => 's',
   prorettype => 'bool', proargtypes => 'oid',
   prosrc => 'pg_collation_is_visible' },
-{ oid => '4191', descr => 'is schema variable visible in search path?',
+{ oid => '4142', descr => 'is schema variable visible in search path?',
   proname => 'pg_variable_is_visible', procost => '10', provolatile => 's',
   prorettype => 'bool', proargtypes => 'oid', prosrc => 'pg_variable_is_visible' },
 


schema-variables-20200320.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh patch - rebase and fix issue reported by Remi - broken usage CREATE
VARIABLE inside PLpgSQL

Regards

Pavel


schema-variables-20200318.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

ne 8. 3. 2020 v 19:12 odesílatel Pavel Stehule 
napsal:

>
>
> so 7. 3. 2020 v 22:15 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>> I fixed the some ugly parts of this patch - now the LET x = DEFAULT, LET
>> x = NULL is processed by more usual way.
>> Statement LET is doesn't switch between STMT_UTILITY and
>> STMT_PLAN_UTILITY like before. It is only STMT_PLAN_UTILITY statement.
>>
>
> I did some cleaning - I mainly replaced CMD_PLAN_UTILITY by CMD_LET
> because there is not another similar statement in queue.
>

 another cleaning - I repleaced CMD_LET by CMD_SELECT_UTILITY. This name is
more illustrative, and little bit cleaned code.

CMD_SELECT_UTILITY is mix of CMD_SELECT and CMD_UTILITY. It allows PREPARE
and parametrizations like CMD_SELECT. But execution is like CMD_UTILITY by
own utility routine with explicit dest receiver setting. This design
doesn't need to introduce new executor and planner nodes (like ModifyTable
and ModifyTablePath).

Regards

Pavel



> Regards
>
> Pavel
>
>
>> Regards
>>
>> Pavel
>>
>


schema-variables-20200313.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

so 7. 3. 2020 v 22:15 odesílatel Pavel Stehule 
napsal:

> Hi
>
> I fixed the some ugly parts of this patch - now the LET x = DEFAULT, LET x
> = NULL is processed by more usual way.
> Statement LET is doesn't switch between STMT_UTILITY and STMT_PLAN_UTILITY
> like before. It is only STMT_PLAN_UTILITY statement.
>

I did some cleaning - I mainly replaced CMD_PLAN_UTILITY by CMD_LET because
there is not another similar statement in queue.

Regards

Pavel


> Regards
>
> Pavel
>


schema-variables-20200308.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

I fixed the some ugly parts of this patch - now the LET x = DEFAULT, LET x
= NULL is processed by more usual way.
Statement LET is doesn't switch between STMT_UTILITY and STMT_PLAN_UTILITY
like before. It is only STMT_PLAN_UTILITY statement.

Regards

Pavel


schema-variables-20200307.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

pá 6. 3. 2020 v 16:44 odesílatel DUVAL REMI  napsal:

> Hello Pavel
>
>
>
> I tested your patch again and I think things are better now, close to
> perfect for me.
>
>
>
> *1)  **Patch review*
>
>
>
> -  We can define CONSTANTs with CREATE IMMUTABLE VARIABLE … I’m
> really pleased with this
>
> -  The previous bug I mentioned to you by private mail (see
> detail below) has been fixed and a non-regression test case has been added
> for it.
>
> -  I’m now able to export a 12.1 database using pg_dump from the
> latest git HEAD (internal version 13).
>
> NB: the condition is “if internal_version < 13 => don’t try to export
> any schema variable”.
>
>
>
> Also I was able to test a use case for a complex Oracle package I migrated
> to PostgreSQL (it has a total of 194 variables and constants in it !).
>
> The Oracle package has been migrated to a PostgreSQL schema with one
> routine per Oracle subprogram.
>
> I’m able to run my business test case using schema variables on those
> routines and it’s giving me the expected result.
>
>
>
> NB: Previous bug was
>
> database1=> CREATE VARIABLE T0_var AS char(14);
> CREATE VARIABLE
> database1=> CREATE IMMUTABLE VARIABLE Taille_var AS numeric DEFAULT 14;
> CREATE VARIABLE
> database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER,
> '0');
> *ERROR:  schema variable "taille_var" is declared IMMUTABLE*
> database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER,
> '0');
> *ERROR:  variable "taille_var" has not valid content*
>
> ð  It’s now fixed !
>
>
>
> *2)  **Regarding documentation *
>
>
>
> It’s pretty good except some small details :
>
> -  sql-createvariable.html => IMMUTABLE parameter : The value of
> *the* variable cannot be changed. => I think an article is needed here
> (the)
>

fixed

-  sql-createvariable.html => ON COMMIT DROP : The ON COMMIT DROP
> clause specifies the bahaviour of  temporary => behaviour
> Also there are “tabs” between words (here between “of” and “temporary”),
> making the paragraph look strange.
>

fixed

> -  sql-createvariable.html => Maybe we should mention that the
> IMMUTABLE parameter (CREATE IMMUTABLE VARIABLE …) is the way to define
> global CONSTANTs, because people will look for a way to create global
> constants in the documentation and it would be good if they can find the
> CONSTANT word in it.
> Also maybe it’s worth mentioning that “schema variables” relates to
> “global variables” (for the same purpose of people searching in the
> documentation).
>
probably it should be somewhere
https://www.postgresql.org/docs/current/plpgsql-porting.html

I wrote note there


> -  sql-altervariable.html => sentence “These restrictions enforce
> that altering the owner doesn't do anything you couldn't do by dropping and
> recreating the variable.“ => not sure I understand this one. Isn’t it
> “does not do anything you could do” instead of “you couln’t do” .. but
> maybe it’s me
>
This sentence is used more times (from alter_view0

To alter the owner, you must also be a direct or indirect member of the new
   owning role, and that role must have CREATE privilege
on
   the view's schema.  (These restrictions enforce that altering the owner
   doesn't do anything you couldn't do by dropping and recreating the view.
   However, a superuser can alter ownership of any view anyway.)


>
>
> Otherwise, this is a really nice feature and I’m looking forward to have
> it in PostgreSQL.
>

Thank you very much

updated patch attached



>
> Thanks a lot
>
>
>
> Duval Rémi
>
>
>
> *De :* Pavel Stehule [mailto:pavel.steh...@gmail.com]
> *Envoyé :* jeudi 5 mars 2020 18:54
> *À :* Asif Rehman 
> *Cc :* DUVAL REMI ; PostgreSQL Hackers <
> pgsql-hackers@lists.postgresql.org>
> *Objet :* Re: proposal: schema variables
>
>
>
>
>
>
>
> čt 5. 3. 2020 v 15:11 odesílatel Asif Rehman 
> napsal:
>
>
>
>
>
> On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule 
> wrote:
>
>
>
>
>
> pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule 
> napsal:
>
>
>
>
>
> čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
> napsal:
>
>
>
> Hi
>
>
>
>
> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should not
> be changed once defined.
> Maybe it's hard to implement and can be implemented

RE: proposal: schema variables

[DUVAL REMI]

Hello Pavel

I tested your patch again and I think things are better now, close to perfect 
for me.


1)  Patch review


-  We can define CONSTANTs with CREATE IMMUTABLE VARIABLE … I’m really 
pleased with this

-  The previous bug I mentioned to you by private mail (see detail 
below) has been fixed and a non-regression test case has been added for it.

-  I’m now able to export a 12.1 database using pg_dump from the latest 
git HEAD (internal version 13).

NB: the condition is “if internal_version < 13 => don’t try to export any 
schema variable”.

Also I was able to test a use case for a complex Oracle package I migrated to 
PostgreSQL (it has a total of 194 variables and constants in it !).
The Oracle package has been migrated to a PostgreSQL schema with one routine 
per Oracle subprogram.
I’m able to run my business test case using schema variables on those routines 
and it’s giving me the expected result.

NB: Previous bug was
database1=> CREATE VARIABLE T0_var AS char(14);
CREATE VARIABLE
database1=> CREATE IMMUTABLE VARIABLE Taille_var AS numeric DEFAULT 14;
CREATE VARIABLE
database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER, '0');
ERROR:  schema variable "taille_var" is declared IMMUTABLE
database1=> LET T0_var = LPAD('999', trunc(Taille_var::NUMERIC)::INTEGER, '0');
ERROR:  variable "taille_var" has not valid content

ð  It’s now fixed !


2)  Regarding documentation

It’s pretty good except some small details :

-  sql-createvariable.html => IMMUTABLE parameter : The value of the 
variable cannot be changed. => I think an article is needed here (the)

-  sql-createvariable.html => ON COMMIT DROP : The ON COMMIT DROP 
clause specifies the bahaviour of  temporary => behaviour
Also there are “tabs” between words (here between “of” and “temporary”), making 
the paragraph look strange.

-  sql-createvariable.html => Maybe we should mention that the 
IMMUTABLE parameter (CREATE IMMUTABLE VARIABLE …) is the way to define global 
CONSTANTs, because people will look for a way to create global constants in the 
documentation and it would be good if they can find the CONSTANT word in it.
Also maybe it’s worth mentioning that “schema variables” relates to “global 
variables” (for the same purpose of people searching in the documentation).

-  sql-altervariable.html => sentence “These restrictions enforce that 
altering the owner doesn't do anything you couldn't do by dropping and 
recreating the variable.“ => not sure I understand this one. Isn’t it “does not 
do anything you could do” instead of “you couln’t do” .. but maybe it’s me

Otherwise, this is a really nice feature and I’m looking forward to have it in 
PostgreSQL.

Thanks a lot

Duval Rémi

De : Pavel Stehule [mailto:pavel.steh...@gmail.com]
Envoyé : jeudi 5 mars 2020 18:54
À : Asif Rehman 
Cc : DUVAL REMI ; PostgreSQL Hackers 

Objet : Re: proposal: schema variables



čt 5. 3. 2020 v 15:11 odesílatel Asif Rehman 
mailto:asifr.reh...@gmail.com>> napsal:


On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule 
mailto:pavel.steh...@gmail.com>> wrote:


pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule 
mailto:pavel.steh...@gmail.com>> napsal:


čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
mailto:pavel.steh...@gmail.com>> napsal:

Hi


3) Any way to define CONSTANTs ?
We already talked a bit about this subject and also Gilles Darold introduces it 
in this mailing-list topic but I'd like to insist on it.
I think it would be nice to have a way to say that a variable should not be 
changed once defined.
Maybe it's hard to implement and can be implemented later, but I just want to 
know if this concern is open.

I played little bit with it and I didn't find any nice solution, but maybe I 
found the solution. I had ideas about some variants, but almost all time I had 
a problem with parser's shifts because all potential keywords are not reserved.

last variant, but maybe best is using keyword WITH

So the syntax can looks like

CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT expression 
] [ WITH [ OPTIONS ] '(' ... ')' ] ]

What do you think about this syntax? It doesn't need any new keyword, and it 
easy to enhance it.

CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);

After some more thinking and because in other patch I support syntax CREATE 
TRANSACTION VARIABLE ... I change my opinion and implemented support for
syntax CREATE IMMUTABLE VARIABLE for define constants.

second try to fix pg_dump

Regards

Pavel


See attached patch

Regards

Pavel


?

Regards

Pavel



Hi Pavel,

I have been reviewing the latest patch (schema-variables-20200229.patch.gz)
and here are few comments:

1- There is a compilation error, when compiled with --with-llvm enabled on
CentOS 7.

llvmjit_expr.c: In function ‘llvm_compile_expr’:
llvmjit_expr.c:

Re: proposal: schema variables

[Pavel Stehule]

čt 5. 3. 2020 v 15:11 odesílatel Asif Rehman 
napsal:

>
>
> On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule 
> wrote:
>
>>
>>
>> pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule 
>> napsal:
>>
>>>
>>>
>>> čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
>>> napsal:
>>>

 Hi


> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should
> not be changed once defined.
> Maybe it's hard to implement and can be implemented later, but I just
> want to know if this concern is open.
>

 I played little bit with it and I didn't find any nice solution, but
 maybe I found the solution. I had ideas about some variants, but almost all
 time I had a problem with parser's shifts because all potential keywords
 are not reserved.

 last variant, but maybe best is using keyword WITH

 So the syntax can looks like

 CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
 expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]

 What do you think about this syntax? It doesn't need any new keyword,
 and it easy to enhance it.

 CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);

>>>
>>> After some more thinking and because in other patch I support syntax
>>> CREATE TRANSACTION VARIABLE ... I change my opinion and implemented support
>>> for
>>> syntax CREATE IMMUTABLE VARIABLE for define constants.
>>>
>>
>> second try to fix pg_dump
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>> See attached patch
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>>

 ?

 Regards

 Pavel



> Hi Pavel,
>
> I have been reviewing the latest patch (schema-variables-20200229.patch.gz)
> and here are few comments:
>
> 1- There is a compilation error, when compiled with --with-llvm enabled on
> CentOS 7.
>
> llvmjit_expr.c: In function ‘llvm_compile_expr’:
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
>  build_EvalXFunc(b, mod, "ExecEvalParamVariable",
>  ^
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
> llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
> type [enabled by default]
> llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
> [enabled by default]
> llvmjit_expr.c:1090:5: warning: passing argument 5 of ‘build_EvalXFuncInt’
> from incompatible pointer type [enabled by default]
> llvmjit_expr.c:60:21: note: expected ‘struct ExprEvalStep *’ but argument
> is of type ‘LLVMValueRef’
>  static LLVMValueRef build_EvalXFuncInt(LLVMBuilderRef b, LLVMModuleRef
> mod,
>  ^
> llvmjit_expr.c:1092:29: error: ‘i’ undeclared (first use in this function)
>  LLVMBuildBr(b, opblocks[i + 1]);
>  ^
> llvmjit_expr.c:1092:29: note: each undeclared identifier is reported only
> once for each function it appears in
> make[2]: *** [llvmjit_expr.o] Error 1
>
>
>
> After looking into it, it turns out that:
> - parameter order was incorrect in build_EvalXFunc()
> - LLVMBuildBr() is using the undeclared variable 'i' whereas it should be
> using 'opno'.
>
>
> 2- Similarly, If the default expression is referencing a function or
> object,
> dependency should be marked, so if the function is not dropped silently.
> otherwise, a cache lookup error will come.
>
> postgres=# create or replace function foofunc() returns timestamp as $$
> begin return now(); end; $$ language plpgsql;
> CREATE FUNCTION
> postgres=# create schema test;
> CREATE SCHEMA
> postgres=# create variable test.v1 as timestamp default foofunc();
> CREATE VARIABLE
> postgres=# drop function foofunc();
> DROP FUNCTION
> postgres=# select test.v1;
> ERROR:  cache lookup failed for function 16437
>
>
Thank you for this analyze and patches. I merged them to attached patch



>
> 3- Variable DEFAULT expression is apparently being evaluated at the time of
> first access. whereas I think that It should be at the time of variable
> creation. consider the following example:
>
> postgres=# create variable test.v2 as timestamp default now();
> CREATE VARIABLE
> postgres=# select now();
>   now
> ---
>  2020-03-05 12:13:29.775373+00
> (1 row)
> postgres=# select test.v2;
>  v2
> 
>  2020-03-05 12:13:37.192317 -- I was expecting this to be earlier than
> the above timestamp.
> (1 row)
>
> postgres=# select test.v2;
>  v2
> 
>  2020-03-05 12:13:37.192317
> (1 

Re: proposal: schema variables

[Asif Rehman]

On Sat, Feb 29, 2020 at 2:10 PM Pavel Stehule 
wrote:

>
>
> pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule 
> napsal:
>
>>
>>
>> čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
>> napsal:
>>
>>>
>>> Hi
>>>
>>>
 3) Any way to define CONSTANTs ?
 We already talked a bit about this subject and also Gilles Darold
 introduces it in this mailing-list topic but I'd like to insist on it.
 I think it would be nice to have a way to say that a variable should
 not be changed once defined.
 Maybe it's hard to implement and can be implemented later, but I just
 want to know if this concern is open.

>>>
>>> I played little bit with it and I didn't find any nice solution, but
>>> maybe I found the solution. I had ideas about some variants, but almost all
>>> time I had a problem with parser's shifts because all potential keywords
>>> are not reserved.
>>>
>>> last variant, but maybe best is using keyword WITH
>>>
>>> So the syntax can looks like
>>>
>>> CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
>>> expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]
>>>
>>> What do you think about this syntax? It doesn't need any new keyword,
>>> and it easy to enhance it.
>>>
>>> CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);
>>>
>>
>> After some more thinking and because in other patch I support syntax
>> CREATE TRANSACTION VARIABLE ... I change my opinion and implemented support
>> for
>> syntax CREATE IMMUTABLE VARIABLE for define constants.
>>
>
> second try to fix pg_dump
>
> Regards
>
> Pavel
>
>
>>
>> See attached patch
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>> ?
>>>
>>> Regards
>>>
>>> Pavel
>>>
>>>
>>>
Hi Pavel,

I have been reviewing the latest patch (schema-variables-20200229.patch.gz)
and here are few comments:

1- There is a compilation error, when compiled with --with-llvm enabled on
CentOS 7.

llvmjit_expr.c: In function ‘llvm_compile_expr’:
llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
type [enabled by default]
 build_EvalXFunc(b, mod, "ExecEvalParamVariable",
 ^
llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
[enabled by default]
llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
type [enabled by default]
llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
[enabled by default]
llvmjit_expr.c:1090:5: warning: initialization from incompatible pointer
type [enabled by default]
llvmjit_expr.c:1090:5: warning: (near initialization for ‘(anonymous)[0]’)
[enabled by default]
llvmjit_expr.c:1090:5: warning: passing argument 5 of ‘build_EvalXFuncInt’
from incompatible pointer type [enabled by default]
llvmjit_expr.c:60:21: note: expected ‘struct ExprEvalStep *’ but argument
is of type ‘LLVMValueRef’
 static LLVMValueRef build_EvalXFuncInt(LLVMBuilderRef b, LLVMModuleRef mod,
 ^
llvmjit_expr.c:1092:29: error: ‘i’ undeclared (first use in this function)
 LLVMBuildBr(b, opblocks[i + 1]);
 ^
llvmjit_expr.c:1092:29: note: each undeclared identifier is reported only
once for each function it appears in
make[2]: *** [llvmjit_expr.o] Error 1



After looking into it, it turns out that:
- parameter order was incorrect in build_EvalXFunc()
- LLVMBuildBr() is using the undeclared variable 'i' whereas it should be
using 'opno'.


2- Similarly, If the default expression is referencing a function or object,
dependency should be marked, so if the function is not dropped silently.
otherwise, a cache lookup error will come.

postgres=# create or replace function foofunc() returns timestamp as $$
begin return now(); end; $$ language plpgsql;
CREATE FUNCTION
postgres=# create schema test;
CREATE SCHEMA
postgres=# create variable test.v1 as timestamp default foofunc();
CREATE VARIABLE
postgres=# drop function foofunc();
DROP FUNCTION
postgres=# select test.v1;
ERROR:  cache lookup failed for function 16437



3- Variable DEFAULT expression is apparently being evaluated at the time of
first access. whereas I think that It should be at the time of variable
creation. consider the following example:

postgres=# create variable test.v2 as timestamp default now();
CREATE VARIABLE
postgres=# select now();
  now
---
 2020-03-05 12:13:29.775373+00
(1 row)
postgres=# select test.v2;
 v2

 2020-03-05 12:13:37.192317 -- I was expecting this to be earlier than the
above timestamp.
(1 row)

postgres=# select test.v2;
 v2

 2020-03-05 12:13:37.192317
(1 row)
postgres=# let test.v2 = default;
LET
postgres=# select test.v2;
 v2

 2020-03-05 12:14:07.538615
(1 row)


To continue my testing of the patch I made few fixes for the above-mentioned
comments. The patch for those changes is attached if it could be of any use.

--
Asif Rehman
Highgo Software (Canada/China/Pakistan)
URL : 

Re: proposal: schema variables

[Pavel Stehule]

pá 28. 2. 2020 v 16:30 odesílatel Pavel Stehule 
napsal:

>
>
> čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
> napsal:
>
>>
>> Hi
>>
>>
>>> 3) Any way to define CONSTANTs ?
>>> We already talked a bit about this subject and also Gilles Darold
>>> introduces it in this mailing-list topic but I'd like to insist on it.
>>> I think it would be nice to have a way to say that a variable should not
>>> be changed once defined.
>>> Maybe it's hard to implement and can be implemented later, but I just
>>> want to know if this concern is open.
>>>
>>
>> I played little bit with it and I didn't find any nice solution, but
>> maybe I found the solution. I had ideas about some variants, but almost all
>> time I had a problem with parser's shifts because all potential keywords
>> are not reserved.
>>
>> last variant, but maybe best is using keyword WITH
>>
>> So the syntax can looks like
>>
>> CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
>> expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]
>>
>> What do you think about this syntax? It doesn't need any new keyword, and
>> it easy to enhance it.
>>
>> CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);
>>
>
> After some more thinking and because in other patch I support syntax
> CREATE TRANSACTION VARIABLE ... I change my opinion and implemented support
> for
> syntax CREATE IMMUTABLE VARIABLE for define constants.
>

second try to fix pg_dump

Regards

Pavel


>
> See attached patch
>
> Regards
>
> Pavel
>
>
>>
>> ?
>>
>> Regards
>>
>> Pavel
>>
>>
>>


schema-variables-20200229.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 27. 2. 2020 v 15:37 odesílatel Pavel Stehule 
napsal:

>
> Hi
>
>
>> 3) Any way to define CONSTANTs ?
>> We already talked a bit about this subject and also Gilles Darold
>> introduces it in this mailing-list topic but I'd like to insist on it.
>> I think it would be nice to have a way to say that a variable should not
>> be changed once defined.
>> Maybe it's hard to implement and can be implemented later, but I just
>> want to know if this concern is open.
>>
>
> I played little bit with it and I didn't find any nice solution, but maybe
> I found the solution. I had ideas about some variants, but almost all time
> I had a problem with parser's shifts because all potential keywords are not
> reserved.
>
> last variant, but maybe best is using keyword WITH
>
> So the syntax can looks like
>
> CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
> expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]
>
> What do you think about this syntax? It doesn't need any new keyword, and
> it easy to enhance it.
>
> CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);
>

After some more thinking and because in other patch I support syntax CREATE
TRANSACTION VARIABLE ... I change my opinion and implemented support for
syntax CREATE IMMUTABLE VARIABLE for define constants.

See attached patch

Regards

Pavel


>
> ?
>
> Regards
>
> Pavel
>
>
>


schema-variables-20200228.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

čt 27. 2. 2020 v 15:59 odesílatel DUVAL REMI  napsal:

> Hello Pavel.
>
>
>
> That looks pretty good to me !
>
>
>
> I’m adding Philippe Beaudoin who was also interested in this topic.
>
>
>
> Recap : We were looking for a way to separate variable from constants in
> the “Schema Variables” proposition from Pavel.
>
> Pavel was saying that there are some limitations regarding the keywords we
> can use, as the community don’t want to introduce too much new keywords in
> Postgres SQL (PL/pgSQL is a different list of keywords).
>
> “CONSTANT” is not a keyword in SQL for Now (though it is one in PL/pgSQL).
>
> Pavel’s syntax allow to use it as a keyword in the “WITH OPTIONS” clause
> that is already supported.
>
> … I think it’s a good idea.
>
>
>
> The list of keywords is defined in : postgresql\src\include\parser\kwlist.h
>
>
>
> Pavel, I saw that in DB2, those variables are called “Global Variables”,
> is it something we can consider changing, or do you prefer to keep using
> the “Schema Variable” name ?
>

It is most hard question. Global variables has sense, but when we will use
it in plpgsql, then this name can be little bit confusing. Personally I
prefer "schema variable" although my opinion is not too strong. This name
more signalize so this is more generic, more database related than some
special kind of plpgsql variables. Now, I think so maybe is better to use
schema variables, because there is different syntax then global temp
tables. Variables are global by design. So in this moment I prefer the name
"schema variables". It can be used as global variables in plpgsql, but it
is one case.

Pavel



>
>
>
> *De :* Pavel Stehule [mailto:pavel.steh...@gmail.com]
> *Envoyé :* jeudi 27 février 2020 15:38
> *À :* DUVAL REMI 
> *Cc :* PostgreSQL Hackers 
> *Objet :* Re: proposal: schema variables
>
>
>
>
>
> Hi
>
>
>
>
> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should not
> be changed once defined.
> Maybe it's hard to implement and can be implemented later, but I just want
> to know if this concern is open.
>
>
>
> I played little bit with it and I didn't find any nice solution, but maybe
> I found the solution. I had ideas about some variants, but almost all time
> I had a problem with parser's shifts because all potential keywords are not
> reserved.
>
>
>
> last variant, but maybe best is using keyword WITH
>
>
>
> So the syntax can looks like
>
>
>
> CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
> expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]
>
>
>
> What do you think about this syntax? It doesn't need any new keyword, and
> it easy to enhance it.
>
>
>
> CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);
>
>
>
> ?
>
>
>
> Regards
>
>
>
> Pavel
>
>
>
>
>

RE: proposal: schema variables

[DUVAL REMI]

Hello Pavel.

That looks pretty good to me !

I’m adding Philippe Beaudoin who was also interested in this topic.

Recap : We were looking for a way to separate variable from constants in the 
“Schema Variables” proposition from Pavel.
Pavel was saying that there are some limitations regarding the keywords we can 
use, as the community don’t want to introduce too much new keywords in Postgres 
SQL (PL/pgSQL is a different list of keywords).
“CONSTANT” is not a keyword in SQL for Now (though it is one in PL/pgSQL).
Pavel’s syntax allow to use it as a keyword in the “WITH OPTIONS” clause that 
is already supported.
… I think it’s a good idea.

The list of keywords is defined in : postgresql\src\include\parser\kwlist.h

Pavel, I saw that in DB2, those variables are called “Global Variables”, is it 
something we can consider changing, or do you prefer to keep using the “Schema 
Variable” name ?


De : Pavel Stehule [mailto:pavel.steh...@gmail.com]
Envoyé : jeudi 27 février 2020 15:38
À : DUVAL REMI 
Cc : PostgreSQL Hackers 
Objet : Re: proposal: schema variables


Hi


3) Any way to define CONSTANTs ?
We already talked a bit about this subject and also Gilles Darold introduces it 
in this mailing-list topic but I'd like to insist on it.
I think it would be nice to have a way to say that a variable should not be 
changed once defined.
Maybe it's hard to implement and can be implemented later, but I just want to 
know if this concern is open.

I played little bit with it and I didn't find any nice solution, but maybe I 
found the solution. I had ideas about some variants, but almost all time I had 
a problem with parser's shifts because all potential keywords are not reserved.

last variant, but maybe best is using keyword WITH

So the syntax can looks like

CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT expression 
] [ WITH [ OPTIONS ] '(' ... ')' ] ]

What do you think about this syntax? It doesn't need any new keyword, and it 
easy to enhance it.

CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);

?

Regards

Pavel

Re: proposal: schema variables

[Pavel Stehule]

Hi


> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should not
> be changed once defined.
> Maybe it's hard to implement and can be implemented later, but I just want
> to know if this concern is open.
>

I played little bit with it and I didn't find any nice solution, but maybe
I found the solution. I had ideas about some variants, but almost all time
I had a problem with parser's shifts because all potential keywords are not
reserved.

last variant, but maybe best is using keyword WITH

So the syntax can looks like

CREATE [ TEMP ] VARIABLE varname [ AS ] type [ NOT NULL ] [ DEFAULT
expression ] [ WITH [ OPTIONS ] '(' ... ')' ] ]

What do you think about this syntax? It doesn't need any new keyword, and
it easy to enhance it.

CREATE VARIABLE foo AS int DEFAULT 10 WITH OPTIONS ( CONSTANT);

?

Regards

Pavel

Re: proposal: schema variables

[Pavel Stehule]

st 26. 2. 2020 v 15:54 odesílatel remi duval  napsal:

> The following review has been posted through the commitfest application:
> make installcheck-world:  not tested
> Implements feature:   tested, passed
> Spec compliant:   tested, failed
> Documentation:tested, failed
>
> Hello Pavel
>
> First thanks for working on this patch cause it might be really helpful
> for those of us trying to migrate PL code between RDBMs.
>
> I tried your patch for migrating an Oracle package body to PL/pgSQL after
> also testing a solution using set_config and current_setting (which works
> but I'm not really satisfied by this workaround solution).
>
> So I compiled latest postgres sources from github on Linux (redhat 7.7)
> using only your patch number 1 (I did not try the second part of the patch).
>
> For my use-case it's working great, performances are excellent (compared
> to other solution for porting "package variables").
> I did not test all the features involved by the patch (especially ALTER
> variable).
>

ALTER VARIABLE is not implemented yet


> I have some feedback however :
>
> 1) Failure when using pg_dump 13 on a 12.1 database
>
> When exporting a 12.1 database using pg_dump from the latest development
> sources I have an error regarding variables export
>
> [pg12@TST-LINUX-PG-03 ~]$ /opt/postgres12/pg12/bin/pg_dump -h localhost
> -p 5432 -U postgres -f dump_pg12.sql database1
> pg_dump: error: query failed: ERROR:  relation "pg_variable" does not exist
> LINE 1: ...og.pg_get_expr(v.vardefexpr,0) as vardefexpr FROM pg_variabl...
>  ^
> pg_dump: error: query was: SELECT v.tableoid, v.oid, v.varname,
> v.vareoxaction, v.varnamespace,
> (SELECT rolname FROM pg_catalog.pg_roles WHERE oid = varowner) AS rolname
> , (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM (SELECT acl, row_n
> FROM
> pg_catalog.unnest(coalesce(v.varacl,pg_catalog.acldefault('V',v.varowner)))
>  WITH ORDINALITY AS perm(acl,row_n)
>  WHERE NOT EXISTS ( SELECT 1 FROM
> pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault('V',v.varowner)))
> AS init(init_acl)
> WHERE acl = init_acl)) as foo) as varacl, ...:
>
> I think that it should have worked anyway cause the documentation states :
> https://www.postgresql.org/docs/current/upgrading.html
> "It is recommended that you use the pg_dump and pg_dumpall programs from
> the newer version of PostgreSQL, to take advantage of enhancements that
> might have been made in these programs." (that's what I did here)
>
> I think there should be a way to avoid dumping the variable if they don't
> exist, should'nt it ?
>

There was a protection against dump 11, but now it should be Postgres 12.
Fixed.


>
> 2) Displaying the variables + completion
> I created 2 variables using :
> CREATE VARIABLE my_pkg.g_dat_deb varchar(11);
> CREATE VARIABLE my_pkg.g_dat_fin varchar(11);
> When I try to display them, I can only see them when prefixing by the
> schema :
> bdd13=> \dV
> "Did not find any schema variables."
> bdd13=> \dV my_pkg.*
>List of variables
>Schema   |  Name  | Type  | Is nullable |
> Default | Owner | Transactional end action
>
> ++---+-+-+---+--
>  my_pkg| g_dat_deb   | character varying(11) | t   | |
> myowner   |
>  my_pkg| g_dat_fin | character varying(11) | t   | |
> myowner   |
> (3 rows)
>

it is ok - it depends on SEARCH_PATH value


> bdd13=> \dV my_pkg
> Did not find any schema variable named "my_pck".
> NB : Using this template, functions are returned, maybe variables should
> also be listed ? (here by querying on "my_pkg%")
> cts_get13=> \dV my_p [TAB]
> => completion using [TAB] key is not working
>
> Is this normal that I cannot see all the variables when not specifying any
> schema ?
> Also the completion works for functions, but not for variable.
> That's just some bonus but it might be good to have it.
>
> I think the way variables are listed using \dV should match with \df for
> querying functions
>

fixed


> 3) Any way to define CONSTANTs ?
> We already talked a bit about this subject and also Gilles Darold
> introduces it in this mailing-list topic but I'd like to insist on it.
> I think it would be nice to have a way to say that a variable should not
> be changed once defined.
> Maybe it's hard to implement and can be implemented later, but I just want
> to know if this concern is open.
>

This topic is open. I tried to play with it. The problem is syntax. When I
try to reproduce syntax from PLpgSQL, then I need to introduce new reserved
keyword. So my initial idea was wrong.
We need to open discussion about implementable syntax. For this moment you
can use a workaround - any schema variable without WRITE right is constant.
Implementation is easy. Design 

Re: proposal: schema variables

[remi duval]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   tested, failed
Documentation:tested, failed

Hello Pavel

First thanks for working on this patch cause it might be really helpful for 
those of us trying to migrate PL code between RDBMs.

I tried your patch for migrating an Oracle package body to PL/pgSQL after also 
testing a solution using set_config and current_setting (which works but I'm 
not really satisfied by this workaround solution).

So I compiled latest postgres sources from github on Linux (redhat 7.7) using 
only your patch number 1 (I did not try the second part of the patch).

For my use-case it's working great, performances are excellent (compared to 
other solution for porting "package variables").
I did not test all the features involved by the patch (especially ALTER 
variable).

I have some feedback however :

1) Failure when using pg_dump 13 on a 12.1 database

When exporting a 12.1 database using pg_dump from the latest development 
sources I have an error regarding variables export 

[pg12@TST-LINUX-PG-03 ~]$ /opt/postgres12/pg12/bin/pg_dump -h localhost -p 5432 
-U postgres -f dump_pg12.sql database1
pg_dump: error: query failed: ERROR:  relation "pg_variable" does not exist
LINE 1: ...og.pg_get_expr(v.vardefexpr,0) as vardefexpr FROM pg_variabl...
 ^
pg_dump: error: query was: SELECT v.tableoid, v.oid, v.varname, v.vareoxaction, 
v.varnamespace, 
(SELECT rolname FROM pg_catalog.pg_roles WHERE oid = varowner) AS rolname
, (SELECT pg_catalog.array_agg(acl ORDER BY row_n) FROM (SELECT acl, row_n 
FROM pg_catalog.unnest(coalesce(v.varacl,pg_catalog.acldefault('V',v.varowner)))
 WITH ORDINALITY AS perm(acl,row_n) 
 WHERE NOT EXISTS ( SELECT 1 FROM 
pg_catalog.unnest(coalesce(pip.initprivs,pg_catalog.acldefault('V',v.varowner)))
 AS init(init_acl) 
WHERE acl = init_acl)) as foo) as varacl, ...:

I think that it should have worked anyway cause the documentation states :
https://www.postgresql.org/docs/current/upgrading.html
"It is recommended that you use the pg_dump and pg_dumpall programs from the 
newer version of PostgreSQL, to take advantage of enhancements that might have 
been made in these programs." (that's what I did here)

I think there should be a way to avoid dumping the variable if they don't 
exist, should'nt it ?

2) Displaying the variables + completion
I created 2 variables using :
CREATE VARIABLE my_pkg.g_dat_deb varchar(11);
CREATE VARIABLE my_pkg.g_dat_fin varchar(11);
When I try to display them, I can only see them when prefixing by the schema :
bdd13=> \dV
"Did not find any schema variables."
bdd13=> \dV my_pkg.*
   List of variables
   Schema   |  Name  | Type  | Is nullable | Default | 
Owner | Transactional end action
++---+-+-+---+--
 my_pkg| g_dat_deb   | character varying(11) | t   | | myowner  
 |
 my_pkg| g_dat_fin | character varying(11) | t   | | 
myowner   |
(3 rows)

bdd13=> \dV my_pkg
Did not find any schema variable named "my_pck".
NB : Using this template, functions are returned, maybe variables should also 
be listed ? (here by querying on "my_pkg%")
cts_get13=> \dV my_p [TAB]
=> completion using [TAB] key is not working

Is this normal that I cannot see all the variables when not specifying any 
schema ?
Also the completion works for functions, but not for variable.
That's just some bonus but it might be good to have it.

I think the way variables are listed using \dV should match with \df for 
querying functions

3) Any way to define CONSTANTs ?
We already talked a bit about this subject and also Gilles Darold introduces it 
in this mailing-list topic but I'd like to insist on it.
I think it would be nice to have a way to say that a variable should not be 
changed once defined.
Maybe it's hard to implement and can be implemented later, but I just want to 
know if this concern is open.


Otherwise the documentation looks good to me.

Regards

Rémi

Re: proposal: schema variables

[Pavel Stehule]

po 10. 2. 2020 v 19:47 odesílatel Pavel Stehule 
napsal:

>
>
> pá 7. 2. 2020 v 17:09 odesílatel Pavel Stehule 
> napsal:
>
>> Hi
>>
>> rebase
>>
>> Regards
>>
>> Pavel
>>
>
> Hi
>
> another rebase, fix \dV statement (for 0001 patch)
>

rebase

Pavel


> Regards
>
> Pavel
>


0001-schema-variables-20200215.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

pá 7. 2. 2020 v 17:09 odesílatel Pavel Stehule 
napsal:

> Hi
>
> rebase
>
> Regards
>
> Pavel
>

Hi

another rebase, fix \dV statement (for 0001 patch)

Regards

Pavel


0002-transactional-variables-20200210.patch.gz
Description: application/gzip


0001-schema-variables-20200210.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

rebase

Regards

Pavel


0002-transactional-variables-20200207.patch.gz
Description: application/gzip


0001-schema-variables-20200207.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

>
>
> 12) I find it rather suspicious that we make decisions in utility.c
> solely based on commandType (whether it's CMD_UTILITY or not). IMO
> it's pretty strange/ugly that T_LetStmt can be both CMD_UTILITY and
> CMD_PLAN_UTILITY:
>
>  case T_LetStmt:
>  {
>  if (pstmt->commandType == CMD_UTILITY)
>  doLetStmtReset(pstmt);
>  else
>  {
>  Assert(pstmt->commandType == CMD_PLAN_UTILITY);
>  doLetStmtEval(pstmt, params, queryEnv, queryString);
>  }
>
>  if (completionTag)
>  strcpy(completionTag, "LET");
>  }
>  break;
>
>
>
It looks strange, but it has sense, because the LET stmt supports reset to
default value.

I can write

1. LET var = DEFAULT;
2. LET var = (query);

In first case I have not any query, that I can assign, and in this case the
LET statement is really only UTILITY.

I did comment there

Regards

Pavel




>
>
> regards
>
> --
> Tomas Vondra  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


0001-schema-variables-20200126.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

st 22. 1. 2020 v 0:41 odesílatel Tomas Vondra 
napsal:

> Hi,
>
> I did a quick review of this patch today, so let me share a couple of
> comments.
>
> Firstly, the patch is pretty large - it has ~270kB. Not the largest
> patch ever, but large. I think breaking the patch into smaller pieces
> would significantly improve the chnce of it getting committed.
>
> Is it possible to break the patch into smaller pieces that could be
> applied incrementally? For example, we could move the "transactional"
> behavior into a separate patch, but it's not clear to me how much code
> would that actually move to that second patch. Any other parts that
> could be moved to a separate patch?
>

I am sending two patches - 0001 - schema variables, 0002 - transactional
variables

>
> I see one of the main contention points was a rather long discussion
> about transactional vs. non-transactional behavior. I agree with Pavel's
> position that the non-transactional behavior should be the default,
> simply because it's better aligned with what the other databases are
> doing (and supporting migrations seems like one of the main use cases
> for this feature).
>
> I do understand it may not be suitable for some other use cases,
> mentioned by Fabien, but IMHO it's fine to require explicit
> specification of transactional behavior. Well, we can't have both as
> default, and I don't think there's an obvious reason why it should be
> the other way around.
>
> Now, a bunch of comments about the code (some of that nitpicking):
>
>
> 1) This hunk in doc/src/sgml/catalogs.sgml still talks about "table
> creation" instead of schema creation:
>
>   
>vartypmod
>int4
>
>
> vartypmod records type-specific data
> supplied at table creation time (for example, the maximum
> length of a varchar column).  It is passed to
> type-specific input functions and length coercion functions.
> The value will generally be -1 for types that do not need
> vartypmod.
>
>   
>

fixed


>
> 2) This hunk in doc/src/sgml/ref/alter_default_privileges.sgml uses
> "role_name" instead of "variable_name"
>
>  GRANT { READ | WRITE | ALL [ PRIVILEGES ] }
>  ON VARIABLES
>  TO { [ GROUP ] role_name
> | PUBLIC } [, ...] [ WITH GRANT OPTION ]
>

I think so this is correct


>
> 3) I find the syntax in create_variable.sgml a bit too over-complicated:
>
> 
> CREATE [ { TEMPORARY | TEMP } ] [ { TRANSACTIONAL | TRANSACTION } ]
> VARIABLE [ IF NOT EXISTS ]  class="parameter">name [ AS ]  class="parameter">data_type ] [ COLLATE  class="parameter">collation ]
>  [ NOT NULL ] [ DEFAULT  class="parameter">default_expr ] [ { ON COMMIT DROP | ON {
> TRANSACTIONAL | TRANSACTION } END RESET } ]
> 
>
> Do we really need both TRANSACTION and TRANSACTIONAL? Why not just one
> that we already have in the grammar (i.e. TRANSACTION)?
>

It was a Philippe's wish - the implementation is simple, and it is similar
like TEMP, TEMPORARY. I have not any opinion about it.


>
> 4) I think we should rename schemavariable.h to schema_variable.h.
>

done


>
> 5) objectaddress.c has extra line after 'break;' in one switch.
>

fixed


>
> 6) The comment is wrong:
>
>  /*
>   * Find the ObjectAddress for a type or domain
>   */
>  static ObjectAddress
>  get_object_address_variable(List *object, bool missing_ok)
>

fixed


>
> 7) I think the code/comments are really inconsistent in talking about
> "variables" and "schema variables". For example in objectaddress.c we do
> these two things:
>
>  case OCLASS_VARIABLE:
>  appendStringInfoString(, "schema variable");
>  break;
>
> vs.
>
>  case DEFACLOBJ_VARIABLE:
>  appendStringInfoString(,
> " on variables");
>  break;
>
> That's going to be confusing for people.
>
>
fixed


>
> 8) I'm rather confused by CMD_PLAN_UTILITY, which seems to be defined
> merely to support LET. I'm not sure why that's necessary (Why wouldn't
> CMD_UTILITY be sufficient?).
>

Currently out utility statements cannot to hold a execution plan, and
cannot be prepared.

so this enhancing is motivated mainly by performance reasons. I would to
allow any SELECT query there, not just expressions only (see a limits of
CALL statement)



> Having to add conditions checking for CMD_PLAN_UTILITY to various places
> in planner.c is rather annoying, and I wonder how likely it's this will
> unnecessarily break external code in extensions etc.
>
>
> 9) This comment in planner.c seems obsolete (not updated to reflect
> addition of the CMD_PLAN_UTILITY check):
>
>/*
> * If this is an INSERT/UPDATE/DELETE, and we're not being called from
> * inheritance_planner, add the ModifyTable node.
> */
>if (parse->commandType != CMD_SELECT && parse->commandType !=
> CMD_PLAN_UTILITY && !inheritance_update)
>

"If this is an INSERT/UPDATE/DELETE," is related to 

Re: proposal: schema variables

[Tomas Vondra]

Hi,

I did a quick review of this patch today, so let me share a couple of
comments.

Firstly, the patch is pretty large - it has ~270kB. Not the largest
patch ever, but large. I think breaking the patch into smaller pieces
would significantly improve the chnce of it getting committed.

Is it possible to break the patch into smaller pieces that could be
applied incrementally? For example, we could move the "transactional"
behavior into a separate patch, but it's not clear to me how much code
would that actually move to that second patch. Any other parts that
could be moved to a separate patch?

I see one of the main contention points was a rather long discussion
about transactional vs. non-transactional behavior. I agree with Pavel's
position that the non-transactional behavior should be the default,
simply because it's better aligned with what the other databases are
doing (and supporting migrations seems like one of the main use cases
for this feature).

I do understand it may not be suitable for some other use cases,
mentioned by Fabien, but IMHO it's fine to require explicit
specification of transactional behavior. Well, we can't have both as
default, and I don't think there's an obvious reason why it should be
the other way around.

Now, a bunch of comments about the code (some of that nitpicking):


1) This hunk in doc/src/sgml/catalogs.sgml still talks about "table
creation" instead of schema creation:

 
  vartypmod
  int4
  
  
   vartypmod records type-specific data
   supplied at table creation time (for example, the maximum
   length of a varchar column).  It is passed to
   type-specific input functions and length coercion functions.
   The value will generally be -1 for types that do not need 
vartypmod.
  
 


2) This hunk in doc/src/sgml/ref/alter_default_privileges.sgml uses
"role_name" instead of "variable_name"

GRANT { READ | WRITE | ALL [ PRIVILEGES ] }
ON VARIABLES
TO { [ GROUP ] role_name | 
PUBLIC } [, ...] [ WITH GRANT OPTION ]


3) I find the syntax in create_variable.sgml a bit too over-complicated:


CREATE [ { TEMPORARY | TEMP } ] [ { TRANSACTIONAL | TRANSACTION } ] VARIABLE [ IF NOT EXISTS ] name [ AS ] data_type ] [ 
COLLATE collation ]
[ NOT NULL ] [ DEFAULT default_expr ] [ { ON COMMIT DROP | ON { 
TRANSACTIONAL | TRANSACTION } END RESET } ]


Do we really need both TRANSACTION and TRANSACTIONAL? Why not just one
that we already have in the grammar (i.e. TRANSACTION)?


4) I think we should rename schemavariable.h to schema_variable.h.


5) objectaddress.c has extra line after 'break;' in one switch.


6) The comment is wrong:

/*
 * Find the ObjectAddress for a type or domain
 */
static ObjectAddress
get_object_address_variable(List *object, bool missing_ok)


7) I think the code/comments are really inconsistent in talking about
"variables" and "schema variables". For example in objectaddress.c we do
these two things:

case OCLASS_VARIABLE:
appendStringInfoString(, "schema variable");
break;

vs.

case DEFACLOBJ_VARIABLE:
appendStringInfoString(,
   " on variables");
break;

That's going to be confusing for people.


8) I'm rather confused by CMD_PLAN_UTILITY, which seems to be defined
merely to support LET. I'm not sure why that's necessary (Why wouldn't
CMD_UTILITY be sufficient?).

Having to add conditions checking for CMD_PLAN_UTILITY to various places
in planner.c is rather annoying, and I wonder how likely it's this will
unnecessarily break external code in extensions etc.


9) This comment in planner.c seems obsolete (not updated to reflect
addition of the CMD_PLAN_UTILITY check):

  /*
   * If this is an INSERT/UPDATE/DELETE, and we're not being called from
   * inheritance_planner, add the ModifyTable node.
   */
  if (parse->commandType != CMD_SELECT && parse->commandType != CMD_PLAN_UTILITY 
&& !inheritance_update)


10) I kinda wonder what happens when a function is used in a WHERE
condition, but it depends on a variable and alsu mutates it on each
call ...


11) I think Query->hasSchemaVariable (in analyze.c) should be renamed to
hasSchemaVariables (which reflects the other fields referring to things
like window functions etc.)


12) I find it rather suspicious that we make decisions in utility.c
solely based on commandType (whether it's CMD_UTILITY or not). IMO
it's pretty strange/ugly that T_LetStmt can be both CMD_UTILITY and
CMD_PLAN_UTILITY:

case T_LetStmt:
{
if (pstmt->commandType == CMD_UTILITY)
doLetStmtReset(pstmt);
else
{
Assert(pstmt->commandType == CMD_PLAN_UTILITY);
doLetStmtEval(pstmt, params, queryEnv, queryString);
}

if (completionTag)
strcpy(completionTag, "LET");
}
break;


13) Not sure why we moved DO_TABLE in 

Re: proposal: schema variables

[Pavel Stehule]

Hi

po 30. 12. 2019 v 21:05 odesílatel Pavel Stehule 
napsal:

> Hi
>
> po 30. 12. 2019 v 17:27 odesílatel Philippe BEAUDOIN 
> napsal:
>
>> The following review has been posted through the commitfest application:
>> make installcheck-world:  tested, passed
>> Implements feature:   tested, passed
>> Spec compliant:   not tested
>> Documentation:tested, failed
>>
>> Hi Pavel,
>>
>> I have tested the latest version of your patch.
>> Both issues I reported are now fixed. And you largely applied my
>> proposals. That's great !
>>
>> I have also spent some time to review more closely the documentation. I
>> will send you a direct mail with an attached file for some minor comments
>> on this topic.
>>
>> Except these documentation remarks to come, I haven't any other issue or
>> suggestion to report.
>> Note that I have not closely looked at the C code itself. But may be some
>> other reviewers have already done that job.
>> If yes, my feeling is that the patch could soon be set as "Ready for
>> commiter".
>>
>> Best regards. Philippe.
>>
>> The new status of this patch is: Waiting on Author
>>
>
> Thank you very much for your comments, and notes. Updated patch attached.
>

rebase



> Regards
>
> Pavel
>
>


schema-variables-20200117.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

po 30. 12. 2019 v 17:27 odesílatel Philippe BEAUDOIN 
napsal:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   not tested
> Documentation:tested, failed
>
> Hi Pavel,
>
> I have tested the latest version of your patch.
> Both issues I reported are now fixed. And you largely applied my
> proposals. That's great !
>
> I have also spent some time to review more closely the documentation. I
> will send you a direct mail with an attached file for some minor comments
> on this topic.
>
> Except these documentation remarks to come, I haven't any other issue or
> suggestion to report.
> Note that I have not closely looked at the C code itself. But may be some
> other reviewers have already done that job.
> If yes, my feeling is that the patch could soon be set as "Ready for
> commiter".
>
> Best regards. Philippe.
>
> The new status of this patch is: Waiting on Author
>

Thank you very much for your comments, and notes. Updated patch attached.

Regards

Pavel


schema-variables-20191230.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Philippe BEAUDOIN]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, failed

Hi Pavel,

I have tested the latest version of your patch.
Both issues I reported are now fixed. And you largely applied my proposals. 
That's great !

I have also spent some time to review more closely the documentation. I will 
send you a direct mail with an attached file for some minor comments on this 
topic.

Except these documentation remarks to come, I haven't any other issue or 
suggestion to report.
Note that I have not closely looked at the C code itself. But may be some other 
reviewers have already done that job.
If yes, my feeling is that the patch could soon be set as "Ready for commiter".

Best regards. Philippe.

The new status of this patch is: Waiting on Author

Re: proposal: schema variables

[Pavel Stehule]

Hi

fresh rebase

Regards

Pavel


schema-variables-20191226.patch.gz
Description: application/gzip

Re: proposal: schema variables

[Pavel Stehule]

Hi

ne 22. 12. 2019 v 13:04 odesílatel Philippe BEAUDOIN 
napsal:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, failed
> Spec compliant:   not tested
> Documentation:tested, failed
>
> Hi Pavel,
>
> First of all, I would like to congratulate you for this great work. This
> patch is really cool. The lack of package variables is sometimes a blocking
> issue for Oracle to Postgres migrations, because the usual emulation with
> GUC is sometimes not enough, in particular when there are security concerns
> or when the database is used in a public cloud.
>
> As I look forward to having this patch commited, I decided to spend some
> time to participate to the review, although I am not a C specialist and I
> have not a good knowledge of the Postgres internals. Here is my report.
>
> A) Installation
>
> The patch applies correctly and the compilation is fine. The "make check"
> doesn't report any issue.
>
> B) Basic usage
>
> I tried some simple schema variables use cases. No problem.
>
> C) The interface
>
> The SQL changes look good to me.
>
> However, in the CREATE VARIABLE command, I would replace the "TRANSACTION"
> word by "TRANSACTIONAL".
>
> I have also tried to replace this word by a ON ROLLBACK clause at the end
> of the statement, like for ON COMMIT, but I have not found a satisfying
> wording to propose.
>

I propose compromise solution - I introduced new not reserved keyword
"TRANSACTIONAL". User can use TRANSACTION or TRANSACTIONAL. It is similar
relation like "TEMP" or "TEMPORAL"


>
> D) Behaviour
>
> I am ok with variables not being transactional by default. That's the most
> simple, the most efficient, it emulates the package variables of other
> RDBMS and it will probably fit the most common use cases.
>
> Note that I am not strongly opposed to having by default transactional
> variables. But I don't know whether this change would be a great work. We
> would have at least to find another keyword in the CREATE VARIABLE
> statement. Something like "NON-TRANSACTIONAL VARIABLE" ?
>
> It is possible to create a NOT NULL variable without DEFAULT. When trying
> to read the variable before a LET statement, one gets an error massage
> saying that the NULL value is not allowed (and the documentation is clear
> about this case). Just for the records, I wondered whether it wouldn't be
> better to forbid a NOT NULL variable creation that wouldn't have a DEFAULT
> value. But finally, I think this behaviour provides a good way to force the
> variable initialisation before its use. So let's keep it as is.
>
> E) ACL and Rights
>
> I played a little bit with the GRANT and REVOKE statements.
>
> I have got an error (Issue 1). The following statement chain:
>   create variable public.sv1 int;
>   grant read on variable sv1 to other_user;
>   drop owned by other_user;
> reports : ERROR:  unexpected object class 4287
>

should be fixed


> I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I
> successfuly performed:
>   alter default privileges in schema public grant read on variables to
> simple_user;
>   alter default privileges in schema public grant write on variables to
> simple_user;
>

should be fixed


> When variables are then created, the grants are properly given.
> And the psql \ddp command perfectly returns:
>  Default access privileges
>   Owner   | Schema | Type |Access privileges
> --++--+-
>  postgres | public |  | simple_user=SW/postgres
> (1 row)
>
> So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this
> new syntax (Issue 2).
>
> BTW, in the ACL, the READ privilege is represented by a S letter. A
> comment in the source reports that the R letter was used in the past for
> rule privilege. Looking at the postgres sources, I see that this privilege
> on rules has been suppressed  in 8.2, so 13 years ago. As this R letter
> would be a so much better choice, I wonder whether it couldn't be reused
> now for this new purpose. Is it important to keep this letter frozen ?
>

I use ACL_READ constant in my patch. The value of ACL_READ is defined
elsewhere. So the changing from S to R should be done by separate patch and
by separate discussion.


> F) Extension
>
> I then created an extension, whose installation script creates a schema
> variable and functions that use it. The schema variable is correctly linked
> to the extension, so that dropping the extension drops the variable.
>
> But there is an issue when dumping the database (Issue 3). The script
> generated by pg_dump includes the CREATE EXTENSION statement as expected
> but also a redundant CREATE VARIABLE statement for the variable that
> belongs to the extension. As a result, one of course gets an error at
> restore time.
>

should be fixed now


> G) Row Level Security
>
> I did a test activating RLS on a table and 

Re: proposal: schema variables

[Pavel Stehule]

Hi

ne 22. 12. 2019 v 13:04 odesílatel Philippe BEAUDOIN 
napsal:

> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, failed
> Spec compliant:   not tested
> Documentation:tested, failed
>
> Hi Pavel,
>
> First of all, I would like to congratulate you for this great work. This
> patch is really cool. The lack of package variables is sometimes a blocking
> issue for Oracle to Postgres migrations, because the usual emulation with
> GUC is sometimes not enough, in particular when there are security concerns
> or when the database is used in a public cloud.
>
> As I look forward to having this patch commited, I decided to spend some
> time to participate to the review, although I am not a C specialist and I
> have not a good knowledge of the Postgres internals. Here is my report.
>
> A) Installation
>
> The patch applies correctly and the compilation is fine. The "make check"
> doesn't report any issue.
>
> B) Basic usage
>
> I tried some simple schema variables use cases. No problem.
>
> C) The interface
>
> The SQL changes look good to me.
>
> However, in the CREATE VARIABLE command, I would replace the "TRANSACTION"
> word by "TRANSACTIONAL".
>

There is not technical problem - the problem is in introduction new keyword
"transactional" that is near to "transaction". I am not sure if it is
practical to have two "similar" keyword and how much the CREATE statement
has to use correct English grammar.

I am not native speaker, so I am not able to see how bad is using
"TRANSACTION" instead "TRANSACTIONAL" in this context. So I see a risk to
have two important (it is not syntactic sugar) similar keywords.

Just I afraid so using TRANSACTIONAL instead just TRANSACTION is not too
user friendly. I have not strong opinion about this - and the
implementation is easy, but I am not feel comfortable with introduction
this keyword.


> I have also tried to replace this word by a ON ROLLBACK clause at the end
> of the statement, like for ON COMMIT, but I have not found a satisfying
> wording to propose.
>



>
> D) Behaviour
>
> I am ok with variables not being transactional by default. That's the most
> simple, the most efficient, it emulates the package variables of other
> RDBMS and it will probably fit the most common use cases.
>
> Note that I am not strongly opposed to having by default transactional
> variables. But I don't know whether this change would be a great work. We
> would have at least to find another keyword in the CREATE VARIABLE
> statement. Something like "NON-TRANSACTIONAL VARIABLE" ?
>

Variables almost everywhere (global user settings - GUC is only one planet
exception) are non transactional by default. I don't see any reason
introduce new different design than is wide used.


> It is possible to create a NOT NULL variable without DEFAULT. When trying
> to read the variable before a LET statement, one gets an error massage
> saying that the NULL value is not allowed (and the documentation is clear
> about this case). Just for the records, I wondered whether it wouldn't be
> better to forbid a NOT NULL variable creation that wouldn't have a DEFAULT
> value. But finally, I think this behaviour provides a good way to force the
> variable initialisation before its use. So let's keep it as is.
>

This is a question - and there are two possibilities

postgres=# do $$
declare x int not null;
begin
  raise notice '%', x;
end;
$$ ;
ERROR:  variable "x" must have a default value, since it's declared NOT NULL
LINE 2: declare x int not null;
  ^

PLpgSQL requires it. But there is not a possibility to enforce future
setting.

So I know so behave of schema variables is little bit different, but I
think so this difference has interesting use case. You can check if the
variable was modified somewhere or not.


> E) ACL and Rights
>
> I played a little bit with the GRANT and REVOKE statements.
>
> I have got an error (Issue 1). The following statement chain:
>   create variable public.sv1 int;
>   grant read on variable sv1 to other_user;
>   drop owned by other_user;
> reports : ERROR:  unexpected object class 4287
>

this is bug and should be fixed


> I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I
> successfuly performed:
>   alter default privileges in schema public grant read on variables to
> simple_user;
>   alter default privileges in schema public grant write on variables to
> simple_user;
>
> When variables are then created, the grants are properly given.
> And the psql \ddp command perfectly returns:
>  Default access privileges
>   Owner   | Schema | Type |Access privileges
> --++--+-
>  postgres | public |  | simple_user=SW/postgres
> (1 row)
>
> So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this
> new syntax (Issue 2).
>
> BTW, in the ACL, the READ 

Re: proposal: schema variables

[Philippe BEAUDOIN]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, failed
Spec compliant:   not tested
Documentation:tested, failed

Hi Pavel,

First of all, I would like to congratulate you for this great work. This patch 
is really cool. The lack of package variables is sometimes a blocking issue for 
Oracle to Postgres migrations, because the usual emulation with GUC is 
sometimes not enough, in particular when there are security concerns or when 
the database is used in a public cloud.

As I look forward to having this patch commited, I decided to spend some time 
to participate to the review, although I am not a C specialist and I have not a 
good knowledge of the Postgres internals. Here is my report.

A) Installation

The patch applies correctly and the compilation is fine. The "make check" 
doesn't report any issue.

B) Basic usage

I tried some simple schema variables use cases. No problem.

C) The interface

The SQL changes look good to me.

However, in the CREATE VARIABLE command, I would replace the "TRANSACTION" word 
by "TRANSACTIONAL".

I have also tried to replace this word by a ON ROLLBACK clause at the end of 
the statement, like for ON COMMIT, but I have not found a satisfying wording to 
propose.

D) Behaviour

I am ok with variables not being transactional by default. That's the most 
simple, the most efficient, it emulates the package variables of other RDBMS 
and it will probably fit the most common use cases.

Note that I am not strongly opposed to having by default transactional 
variables. But I don't know whether this change would be a great work. We would 
have at least to find another keyword in the CREATE VARIABLE statement. 
Something like "NON-TRANSACTIONAL VARIABLE" ?

It is possible to create a NOT NULL variable without DEFAULT. When trying to 
read the variable before a LET statement, one gets an error massage saying that 
the NULL value is not allowed (and the documentation is clear about this case). 
Just for the records, I wondered whether it wouldn't be better to forbid a NOT 
NULL variable creation that wouldn't have a DEFAULT value. But finally, I think 
this behaviour provides a good way to force the variable initialisation before 
its use. So let's keep it as is.

E) ACL and Rights

I played a little bit with the GRANT and REVOKE statements. 

I have got an error (Issue 1). The following statement chain:
  create variable public.sv1 int;
  grant read on variable sv1 to other_user;
  drop owned by other_user;
reports : ERROR:  unexpected object class 4287

I then tried to use DEFAULT PRIVILEGES. Despite this is not documented, I 
successfuly performed:
  alter default privileges in schema public grant read on variables to 
simple_user;
  alter default privileges in schema public grant write on variables to 
simple_user;

When variables are then created, the grants are properly given.
And the psql \ddp command perfectly returns:
 Default access privileges
  Owner   | Schema | Type |Access privileges
--++--+-
 postgres | public |  | simple_user=SW/postgres
(1 row)

So the ALTER DEFAULT PRIVILEGES documentation chapter has to reflect this new 
syntax (Issue 2).

BTW, in the ACL, the READ privilege is represented by a S letter. A comment in 
the source reports that the R letter was used in the past for rule privilege. 
Looking at the postgres sources, I see that this privilege on rules has been 
suppressed  in 8.2, so 13 years ago. As this R letter would be a so much better 
choice, I wonder whether it couldn't be reused now for this new purpose. Is it 
important to keep this letter frozen ?

F) Extension

I then created an extension, whose installation script creates a schema 
variable and functions that use it. The schema variable is correctly linked to 
the extension, so that dropping the extension drops the variable.

But there is an issue when dumping the database (Issue 3). The script generated 
by pg_dump includes the CREATE EXTENSION statement as expected but also a 
redundant CREATE VARIABLE statement for the variable that belongs to the 
extension. As a result, one of course gets an error at restore time.

G) Row Level Security

I did a test activating RLS on a table and creating a POLICY that references a 
schema variable in its USING and WITH CHECK clauses. Everything worked fine.

H) psql

A \dV meta-command displays all the created variables.
I would change a little bit the provided view. More precisely I would:
- rename "Constraint" into "Is nullable" and report it as a boolean
- rename "Special behave" into "Is transactional" and report it as a boolean
- change the order of columns so to have:
Schema | Name | Type | Is nullable | Default | Owner | Is transactional | 
Transaction end action
"Is nullable" being aside "Default"

I) Performance

I just quickly looked at the performance, and 

Re: proposal: schema variables

[Fabrízio Mello]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

1) There are some errors applying the patch against the current master:

fabrizio@macanudo:/d/postgresql (master) 
$ git apply /tmp/schema-variables-poc-180429-01-diff
/tmp/schema-variables-poc-180429-01-diff:2305: trailing whitespace.
 
/tmp/schema-variables-poc-180429-01-diff:2317: indent with spaces.
We can support UPDATE and SELECT commands on variables.
/tmp/schema-variables-poc-180429-01-diff:2319: indent with spaces.
possible syntaxes:
/tmp/schema-variables-poc-180429-01-diff:2321: indent with spaces.
-- there can be a analogy with record functions
/tmp/schema-variables-poc-180429-01-diff:2322: indent with spaces.
SELECT varname;
warning: squelched 14 whitespace errors
warning: 19 lines add whitespace errors.


2) There are some warnings when during build process

schemavar.c:383:18: warning: expression which evaluates to zero treated as a 
null pointer constant of type 'HeapTuple' (aka 'struct HeapTupleData *')
  [-Wnon-literal-null-conversion]
HeapTuple   tp = InvalidOid;
 ^~
../../../src/include/postgres_ext.h:36:21: note: expanded from macro 
'InvalidOid'
#define InvalidOid  ((Oid) 0)
^
1 warning generated.
tab-complete.c:1268:21: warning: initialization from incompatible pointer type 
[-Wincompatible-pointer-types]
  {"VARIABLE", NULL, _for_list_of_variables},
 ^
tab-complete.c:1268:21: note: (near initialization for 
‘words_after_create[48].vquery’)
llvmjit_expr.c: In function ‘llvm_compile_expr’:
llvmjit_expr.c:253:3: warning: enumeration value ‘EEOP_PARAM_SCHEMA_VARIABLE’ 
not handled in switch [-Wswitch]
   switch (opcode)
   ^

Re: proposal: schema variables

[Pavel Stehule]

2018-03-13 10:54 GMT+01:00 Pavel Luzanov :

> Pavel Stehule wrote
> > Now, there is one important question - storage - Postgres stores all
> > objects to files - only memory storage is not designed yet. This is part,
> > where I need a help.
>
> O, I do not feel confident in such questions.
> May be some ideas you can get from extension with similar functionality:
> https://github.com/postgrespro/pg_variables


Unfortunately not - it doesn't implement this functionality

Regards

Pavel


>
>
> -
> Pavel Luzanov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
>
> --
> Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-
> f1928748.html
>
>

Re: proposal: schema variables

[Pavel Luzanov]

Pavel Stehule wrote
> Now, there is one important question - storage - Postgres stores all
> objects to files - only memory storage is not designed yet. This is part,
> where I need a help.

O, I do not feel confident in such questions.
May be some ideas you can get from extension with similar functionality:
https://github.com/postgrespro/pg_variables

-
Pavel Luzanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html