Re: Schema variables - new implementation for Postgres 15

Hi,

I am returning back to implementation of schema variables. The schema
variables can be used as an alternative to package variables (Oracle's
PL/SQL or ADA). The schema variables can be used as fast and safe storage
of session information for RLS too.

The previous implementation had not cleanly implemented execution of the
LET statement. It was something between query and utility, and although it
was working - it was out of Postgres concept (with different implementation
of queries and utilities).

I totally rewrote the implementation of the LET statement. I prepared two
variants:

First variant is based on the introduction of the new command type CMD_LET
and new very small executor node SetVariable (this is a very very reduced
analogy of ModifyTable node). The code is consistent and what is important
- the LET statement can be prepared. The execution is relatively fast from
PLpgSQL too. Without any special support the execution has the same speed
like non simple queries. The statement reuses an execution plan, but
simple execution is not supported.

Second variant is implemented like a classic utility command. There is not
any surprise. It is shorter, simple, but the LET statement cannot be
prepared (this is the limit of all utility statements). Without special
support in PLpgSQL the execution is about 10x slower than the execution of
the first variant. But there is a new possibility of using the main parser
from PLpgSQL (implemented by Tom for new implementation of assign statement
in pg 14), and then this support in plpgsql requires only a few lines).
When the statement LET is explicitly supported by PLpgSQL, then execution
is very fast (the speed is comparable with the speed of the assign
statement) - it is about 10x faster than the first variant.

I tested code

do $$
declare x int ;
begin
for i in 1..1000000
loop
let ooo = i;
end loop;
end;
$$;

variant 1 .. 1500 ms
variant 2 with PLpgSQL support .. 140 ms
variant 2 without PLpgSQL support 9000 ms

The slower speed of the first variant from PLpgSQL can be fixed. But for
this moment, the speed is good enough. This is the worst case, because in
the first variant LET statement cannot use optimization for simple query
evaluation (now).

Now I think so implementation is significantly cleaner, and I hope so it
will be more acceptable for committers.

I am starting a new thread, because this is a new implementation, and
because I am sending two alternative implementations of one functionality.

Comments, notes, objections?

Regards

Pavel

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
--------------------------------------------------
do $$
declare x int ;
begin
for i in 1..1000000
loop
let ooo = i;
end loop;
end;
$$;

variant 1 .. 1500 ms
variant 2 with PLpgSQL support .. 140 ms
variant 2 without PLpgSQL support 9000 ms
--------------------------------------------------

That's impressive! But 1 million times of variable assignment took only 140 ms? It's that one assignment took only 140 nanosecond, which is near one DRAM access? Can PL/pgSQL processing be really so fast?

Regards
Takayuki Tsunakawa

čt 15. 4. 2021 v 18:02 odesílatel tsunakawa(dot)takay(at)fujitsu(dot)com <
tsunakawa(dot)takay(at)fujitsu(dot)com> napsal:

> From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>
> --------------------------------------------------
>
> do $$
>
> declare x int ;
>
> begin
>
> for i in 1..1000000
>
> loop
>
> let ooo = i;
>
> end loop;
>
> end;
>
> $$;
>
>
>
> variant 1 .. 1500 ms
>
> variant 2 with PLpgSQL support .. 140 ms
>
> variant 2 without PLpgSQL support 9000 ms
>
> --------------------------------------------------
>
>
>
>
>
> That's impressive! But 1 million times of variable assignment took only
> 140 ms? It's that one assignment took only 140 nanosecond, which is near
> one DRAM access? Can PL/pgSQL processing be really so fast?
>

In this case the PLpgSQL can be very fast - and after changes in pg 13, the
PLpgSQL is not significantly slower than Lua or than PHP.

Every body can repeat these tests - I did it on my Lenovo T520 notebook

Pavel

>
>
>
> Regards
>
> Takayuki Tsunakawa
>
>
>

čt 15. 4. 2021 v 10:42 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi,
>
> I am returning back to implementation of schema variables. The schema
> variables can be used as an alternative to package variables (Oracle's
> PL/SQL or ADA). The schema variables can be used as fast and safe storage
> of session information for RLS too.
>
> The previous implementation had not cleanly implemented execution of the
> LET statement. It was something between query and utility, and although it
> was working - it was out of Postgres concept (with different implementation
> of queries and utilities).
>
> I totally rewrote the implementation of the LET statement. I prepared two
> variants:
>
> First variant is based on the introduction of the new command type CMD_LET
> and new very small executor node SetVariable (this is a very very reduced
> analogy of ModifyTable node). The code is consistent and what is important
> - the LET statement can be prepared. The execution is relatively fast from
> PLpgSQL too. Without any special support the execution has the same speed
> like non simple queries. The statement reuses an execution plan, but
> simple execution is not supported.
>
> Second variant is implemented like a classic utility command. There is not
> any surprise. It is shorter, simple, but the LET statement cannot be
> prepared (this is the limit of all utility statements). Without special
> support in PLpgSQL the execution is about 10x slower than the execution of
> the first variant. But there is a new possibility of using the main parser
> from PLpgSQL (implemented by Tom for new implementation of assign statement
> in pg 14), and then this support in plpgsql requires only a few lines).
> When the statement LET is explicitly supported by PLpgSQL, then execution
> is very fast (the speed is comparable with the speed of the assign
> statement) - it is about 10x faster than the first variant.
>
> I tested code
>
> do $$
> declare x int ;
> begin
> for i in 1..1000000
> loop
> let ooo = i;
> end loop;
> end;
> $$;
>
> variant 1 .. 1500 ms
> variant 2 with PLpgSQL support .. 140 ms
> variant 2 without PLpgSQL support 9000 ms
>
> The slower speed of the first variant from PLpgSQL can be fixed. But for
> this moment, the speed is good enough. This is the worst case, because in
> the first variant LET statement cannot use optimization for simple query
> evaluation (now).
>
> Now I think so implementation is significantly cleaner, and I hope so it
> will be more acceptable for committers.
>
> I am starting a new thread, because this is a new implementation, and
> because I am sending two alternative implementations of one functionality.
>
> Comments, notes, objections?
>
>
I am sending only one patch and I assign this thread to commitfest
application

Regards

Pavel

> Regards
>
> Pavel
>
>
>

On Thu, Apr 15, 2021, at 10:42, Pavel Stehule wrote:
> *Attachments:*
> * schema-variables-v-execnode-2021-01.patch
> * schema-variables-v-utility-2021-01.patch

Applications are currently know to be misusing set_config()+current_setting() to pass information in a session or transaction.

Such users might be interested in using Schema variables as a better replacement.

However, since set_config() is transactional, it can't be used as a drop-in replacement:

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

I think the "The content of a variable is not transactional." part is therefore a bad idea.

Another pattern is to use TEMP TABLEs to pass around information in a session or transaction.
If the LET command would be transactional, it could be used as a drop-in replacement for such use-cases as well.

Furthermore, I think a non-transactional LET command would be insidious,
since it looks like any other SQL command, all of which are transactional.
(The ones that aren't such as REINDEX CONCURRENTLY will properly throw an error if run inside a transaction block.)

A non-transactional LET command IMO would be non-SQL-idiomatic and non-intuitive.

/Joel

pá 16. 4. 2021 v 8:07 odesílatel Joel Jacobson <joel(at)compiler(dot)org> napsal:

> On Thu, Apr 15, 2021, at 10:42, Pavel Stehule wrote:
>
> *Attachments:*
>
> - schema-variables-v-execnode-2021-01.patch
> - schema-variables-v-utility-2021-01.patch
>
>
> Applications are currently know to be misusing
> set_config()+current_setting() to pass information in a session or
> transaction.
>
> Such users might be interested in using Schema variables as a better
> replacement.
>
> However, since set_config() is transactional, it can't be used as a
> drop-in replacement:
>
> + <para>
> + The value of a schema variable is local to the current session.
> Retrieving
> + a variable's value returns either a NULL or a default value, unless
> its value
> + is set to something else in the current session with a LET command.
> The content
> + of a variable is not transactional. This is the same as in regular
> variables
> + in PL languages.
> + </para>
>
> I think the "The content of a variable is not transactional." part is
> therefore a bad idea.
>
> Another pattern is to use TEMP TABLEs to pass around information in a
> session or transaction.
> If the LET command would be transactional, it could be used as a drop-in
> replacement for such use-cases as well.
>
> Furthermore, I think a non-transactional LET command would be insidious,
> since it looks like any other SQL command, all of which are transactional.
> (The ones that aren't such as REINDEX CONCURRENTLY will properly throw an
> error if run inside a transaction block.)
>
> A non-transactional LET command IMO would be non-SQL-idiomatic and
> non-intuitive.
>

I am sorry, but in this topic we have different opinions. The variables in
PLpgSQL are not transactional too (same is true in Perl, Python, ...).
Session variables in Oracle, MS SQL, DB2, MySQL are not transactional too.
My primary focus is PLpgSQL - and I would like to use schema variables as
global plpgsql variables (from PLpgSQL perspective) - that means in
Postgres's perspective session variables. But in Postgres, I have to write
features that will work with others PL too - PLPython, PLPerl, ...
Statement SET in ANSI/SQL standard (SQL/PSM) doesn't expect transactional
behaviour for variables too. Unfortunately SET keyword is used in Postgres
for GUC, and isn't possible to reuse without a compatibility break.

The PostgreSQL configuration is transactional, but it is a different
feature designed for different purposes. Using GUC like session variables
is just a workaround. It can be useful for some cases, sure. But it is not
usual behaviour. And for other cases the transactional behaviour is not
practical. Schema variables are not replacement of GUC, schema variables
are not replacement of temporal tables. There is a prepared patch for
global temp tables. I hope so this patch can be committed to Postgres 15.
Global temp tables fixes almost all disadvantages of temporary tables in
Postgres. So the schema variable is not a one row table. It is a different
creature - designed to support the server's side procedural features.

I have prepared a patch that allows non default transactional behaviour
(but this behaviour should not be default - I didn't design schema
variables as temp tables replacement). This patch increases the length of
the current patch about 1/4, and I have enough work with rebasing with the
current patch, so I didn't send it to commitfest now. If schema variables
will be inside core, this day I'll send the patch that allows transactional
behaviour for schema variables - I promise.

Regards

Pavel

> /Joel
>
>
>
>

From: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
--------------------------------------------------
I am sorry, but in this topic we have different opinions. The variables in PLpgSQL are not transactional too (same is true in Perl, Python, ...). Session variables in Oracle, MS SQL, DB2, MySQL are not transactional too. My primary focus is PLpgSQL - and I would like to use schema variables as global plpgsql variables (from PLpgSQL perspective) - that means in Postgres's perspective session variables. But in Postgres, I have to write features that will work with others PL too - PLPython, PLPerl, ... Statement SET in ANSI/SQL standard (SQL/PSM) doesn't expect transactional behaviour for variables too. Unfortunately SET keyword is used in Postgres for GUC, and isn't possible to reuse without a compatibility break.

The PostgreSQL configuration is transactional, but it is a different feature designed for different purposes. Using GUC like session variables is just a workaround. It can be useful for some cases, sure. But it is not usual behaviour. And for other cases the transactional behaviour is not practical. Schema variables are not replacement of GUC, schema variables are not replacement of temporal tables. There is a prepared patch for global temp tables. I hope so this patch can be committed to Postgres 15. Global temp tables fixes almost all disadvantages of temporary tables in Postgres. So the schema variable is not a one row table. It is a different creature - designed to support the server's side procedural features.
--------------------------------------------------

+1
I understand (and wish) this feature is intended to ease migration from Oracle PL/SQL, which will further increase the popularity of Postgres. So, the transactional behavior is not necessary unless Oracle has such a feature.

Furthermore, Postgres already has some non-transactonal SQL commands. So, I don't think we need to reject non-transactional LET.

* Sequence operation: SELECT nextval/setval
* SET [SESSION]
* SET ROLE
* SET SESSION AUTHORIZATION

--------------------------------------------------
I have prepared a patch that allows non default transactional behaviour (but this behaviour should not be default - I didn't design schema variables as temp tables replacement). This patch increases the length of the current patch about 1/4, and I have enough work with rebasing with the current patch, so I didn't send it to commitfest now. If schema variables will be inside core, this day I'll send the patch that allows transactional behaviour for schema variables - I promise.
--------------------------------------------------

I prefer the simpler, targeted one without transactional behavior initially, because added complexity might prevent this feature from being committed in PG 15.

Regards
Takayuki Tsunakawa

Hi

only rebase

Regards

Pavel

st 12. 5. 2021 v 6:17 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi
>
> only rebase
>

second try - rebase after serial_scheduler remove

Regards

Pavel

> Regards
>
> Pavel
>

st 12. 5. 2021 v 7:37 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> st 12. 5. 2021 v 6:17 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> napsal:
>
>> Hi
>>
>> only rebase
>>
>
> second try - rebase after serial_scheduler remove
>

only rebase

Regards

Pavel

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

Hi

rebase

Regards

Pavel

so 12. 6. 2021 v 8:00 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi
>
> rebase
>
>
rebase only

Regards

Pavel

Regards
>
> Pavel
>

Hi,

Review resume:

This patch implements Schema Variables that are database objects that
can hold a single or composite value following the data type used at
variable declaration. Schema variables, like relations, exist within a
schema and their access is controlled via GRANT and REVOKE commands. The
schema variable can be created by the CREATE VARIABLE command, altered
using ALTER VARIABLE and removed using DROP VARIABLE.

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.

The patch apply with the patch command without problem and compilation
reports no warning or errors. Regression tests pass successfully using
make check or make installcheck
It also includes all documentation and regression tests.

Performances are near the set of plpgsql variable settings which is
impressive:

do $$
declare var1 int ; i int;
begin
  for i in 1..1000000
  loop
    var1 := i;
  end loop;
end;
$$;
DO
Time: 71,515 ms

CREATE VARIABLE var1 AS integer;
do $$
declare i int ;
begin
  for i in 1..1000000
  loop
    let var1 = i;
  end loop;
end;
$$;
DO
Time: 94,658 ms

There is just one thing that puzzles me.We can use :

    CREATE VARIABLE var1 AS date NOT NULL;
    postgres=# SELECT var1;
    ERROR:  null value is not allowed for NOT NULL schema variable "var1"

which I understand and is the right behavior. But if we use:

    CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
    postgres=# SELECT var1;
    ERROR:  null value is not allowed for NOT NULL schema variable "var1"
    DETAIL:  The schema variable was not initialized yet.
    postgres=# LET var1=current_date;
    ERROR:  schema variable "var1" is declared IMMUTABLE

It should probably be better to not allow NOT NULL when IMMUTABLE is
used because the variable can not be used at all.  Also probably
IMMUTABLE without a DEFAULT value should also be restricted as it makes
no sens. If the user wants the variable to be NULL he must use DEFAULT
NULL. This is just a though, the above error messages are explicit and
the user can understand what wrong declaration he have done.

Except that I think this patch is ready for committers, so if there is
no other opinion in favor of restricting the use of IMMUTABLE with NOT
NULL and DEFAULT I will change the status to ready for committers.

--
Gilles Darold
http://www.darold.net/

so 28. 8. 2021 v 11:57 odesílatel Gilles Darold <gilles(at)darold(dot)net> napsal:

> Hi,
>
> Review resume:
>
>
> This patch implements Schema Variables that are database objects that can
> hold a single or composite value following the data type used at variable
> declaration. Schema variables, like relations, exist within a schema and
> their access is controlled via GRANT and REVOKE commands. The schema
> variable can be created by the CREATE VARIABLE command, altered using ALTER
> VARIABLE and removed using DROP VARIABLE.
>
> 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.
>
> The patch apply with the patch command without problem and compilation
> reports no warning or errors. Regression tests pass successfully using make
> check or make installcheck
> It also includes all documentation and regression tests.
>
> Performances are near the set of plpgsql variable settings which is
> impressive:
>
> do $$
> declare var1 int ; i int;
> begin
> for i in 1..1000000
> loop
> var1 := i;
> end loop;
> end;
> $$;
> DO
> Time: 71,515 ms
>
> CREATE VARIABLE var1 AS integer;
> do $$
> declare i int ;
> begin
> for i in 1..1000000
> loop
> let var1 = i;
> end loop;
> end;
> $$;
> DO
> Time: 94,658 ms
>
> There is just one thing that puzzles me. We can use :
>
> CREATE VARIABLE var1 AS date NOT NULL;
> postgres=# SELECT var1;
> ERROR: null value is not allowed for NOT NULL schema variable "var1"
>
> which I understand and is the right behavior. But if we use:
>
> CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
> postgres=# SELECT var1;
> ERROR: null value is not allowed for NOT NULL schema variable "var1"
> DETAIL: The schema variable was not initialized yet.
> postgres=# LET var1=current_date;
> ERROR: schema variable "var1" is declared IMMUTABLE
>
> It should probably be better to not allow NOT NULL when IMMUTABLE is used
> because the variable can not be used at all. Also probably IMMUTABLE
> without a DEFAULT value should also be restricted as it makes no sens. If
> the user wants the variable to be NULL he must use DEFAULT NULL. This is
> just a though, the above error messages are explicit and the user can
> understand what wrong declaration he have done.
>

I thought about this case, and I have one scenario, where this behaviour
can be useful. When the variable is declared as IMMUTABLE NOT NULL without
not null default, then any access to the content of the variable has to
fail. I think it can be used for detection, where and when the variable is
first used. So this behavior is allowed just because I think, so this
feature can be interesting for debugging. If this idea is too strange, I
have no problem to disable this case.

Regards

Pavel

>
> Except that I think this patch is ready for committers, so if there is no
> other opinion in favor of restricting the use of IMMUTABLE with NOT NULL
> and DEFAULT I will change the status to ready for committers.
>
> --
> Gilles Daroldhttp://www.darold.net/
>
>

Hi

so 28. 8. 2021 v 11:57 odesílatel Gilles Darold <gilles(at)darold(dot)net> napsal:

> Hi,
>
> Review resume:
>
>
> This patch implements Schema Variables that are database objects that can
> hold a single or composite value following the data type used at variable
> declaration. Schema variables, like relations, exist within a schema and
> their access is controlled via GRANT and REVOKE commands. The schema
> variable can be created by the CREATE VARIABLE command, altered using ALTER
> VARIABLE and removed using DROP VARIABLE.
>
> 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.
>
> The patch apply with the patch command without problem and compilation
> reports no warning or errors. Regression tests pass successfully using make
> check or make installcheck
> It also includes all documentation and regression tests.
>
> Performances are near the set of plpgsql variable settings which is
> impressive:
>
> do $$
> declare var1 int ; i int;
> begin
> for i in 1..1000000
> loop
> var1 := i;
> end loop;
> end;
> $$;
> DO
> Time: 71,515 ms
>
> CREATE VARIABLE var1 AS integer;
> do $$
> declare i int ;
> begin
> for i in 1..1000000
> loop
> let var1 = i;
> end loop;
> end;
> $$;
> DO
> Time: 94,658 ms
>
> There is just one thing that puzzles me. We can use :
>
> CREATE VARIABLE var1 AS date NOT NULL;
> postgres=# SELECT var1;
> ERROR: null value is not allowed for NOT NULL schema variable "var1"
>
> which I understand and is the right behavior. But if we use:
>
> CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
> postgres=# SELECT var1;
> ERROR: null value is not allowed for NOT NULL schema variable "var1"
> DETAIL: The schema variable was not initialized yet.
> postgres=# LET var1=current_date;
> ERROR: schema variable "var1" is declared IMMUTABLE
>
> It should probably be better to not allow NOT NULL when IMMUTABLE is used
> because the variable can not be used at all. Also probably IMMUTABLE
> without a DEFAULT value should also be restricted as it makes no sens. If
> the user wants the variable to be NULL he must use DEFAULT NULL. This is
> just a though, the above error messages are explicit and the user can
> understand what wrong declaration he have done.
>

I wrote a check that disables this case. Please, see the attached patch. I
agree, so this case is confusing, and it is better to disable it.

Regards

Pavel

> Except that I think this patch is ready for committers, so if there is no
> other opinion in favor of restricting the use of IMMUTABLE with NOT NULL
> and DEFAULT I will change the status to ready for committers.
>
> --
> Gilles Daroldhttp://www.darold.net/
>
>

Le 08/09/2021 à 13:41, Pavel Stehule a écrit :
> Hi
>
> so 28. 8. 2021 v 11:57 odesílatel Gilles Darold <gilles(at)darold(dot)net
> <mailto:gilles(at)darold(dot)net>> napsal:
>
> Hi,
>
> Review resume:
>
>
> This patch implements Schema Variables that are database objects
> that can hold a single or composite value following the data type
> used at variable declaration. Schema variables, like relations,
> exist within a schema and their access is controlled via GRANT and
> REVOKE commands. The schema variable can be created by the CREATE
> VARIABLE command, altered using ALTER VARIABLE and removed using
> DROP VARIABLE.
>
> 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.
>
>
> The patch apply with the patch command without problem and
> compilation reports no warning or errors. Regression tests pass
> successfully using make check or make installcheck
> It also includes all documentation and regression tests.
>
> Performances are near the set of plpgsql variable settings which
> is impressive:
>
> do $$
> declare var1 int ; i int;
> begin
>   for i in 1..1000000
>   loop
>     var1 := i;
>   end loop;
> end;
> $$;
> DO
> Time: 71,515 ms
>
> CREATE VARIABLE var1 AS integer;
> do $$
> declare i int ;
> begin
>   for i in 1..1000000
>   loop
>     let var1 = i;
>   end loop;
> end;
> $$;
> DO
> Time: 94,658 ms
>
> There is just one thing that puzzles me.We can use :
>
>     CREATE VARIABLE var1 AS date NOT NULL;
>     postgres=# SELECT var1;
>     ERROR:  null value is not allowed for NOT NULL schema variable
> "var1"
>
> which I understand and is the right behavior. But if we use:
>
>     CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
>     postgres=# SELECT var1;
>     ERROR:  null value is not allowed for NOT NULL schema variable
> "var1"
>     DETAIL:  The schema variable was not initialized yet.
>     postgres=# LET var1=current_date;
>     ERROR:  schema variable "var1" is declared IMMUTABLE
>
> It should probably be better to not allow NOT NULL when IMMUTABLE
> is used because the variable can not be used at all.  Also
> probably IMMUTABLE without a DEFAULT value should also be
> restricted as it makes no sens. If the user wants the variable to
> be NULL he must use DEFAULT NULL. This is just a though, the above
> error messages are explicit and the user can understand what wrong
> declaration he have done.
>
>
> I wrote a check that disables this case.  Please, see the attached
> patch. I agree, so this case is confusing, and it is better to disable it.
>

Great, I also think that this is better to not confuse the user.

    postgres=# CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
    ERROR:  IMMUTABLE NOT NULL variable requires default expression

Working as expected. I have moved the patch to "Ready for committers".
Thanks for this feature.

--
Gilles Darold
http://www.darold.net/

Hi

Great, I also think that this is better to not confuse the user.
>
> postgres=# CREATE IMMUTABLE VARIABLE var1 AS date NOT NULL;
> ERROR: IMMUTABLE NOT NULL variable requires default expression
>
> Working as expected. I have moved the patch to "Ready for committers".
> Thanks for this feature.
>

Thank you very much

Pavel

> --
> Gilles Daroldhttp://www.darold.net/
>
>

Hi

fresh rebase

Regards

Pavel

> [schema-variables-20210909.patch]

Hi Pavel,

The patch applies and compiles fine but 'make check' for the
assert-enabled fails on 131 out of 210 tests.

(while compiling HEAD checks run without errors for both assert-disabled
and assert-enabled)

Erik Rijkers

test tablespace ... ok 303 ms
parallel group (20 tests): oid char pg_lsn int2 varchar txid int4
regproc uuid float4 text name money boolean bit float8 int8 enum numeric
rangetypes
boolean ... ok 112 ms
char ... ok 57 ms
name ... ok 106 ms
varchar ... ok 74 ms
text ... ok 106 ms
int2 ... ok 73 ms
int4 ... ok 92 ms
int8 ... ok 130 ms
oid ... ok 55 ms
float4 ... ok 102 ms
float8 ... ok 126 ms
bit ... ok 124 ms
numeric ... ok 362 ms
txid ... ok 87 ms
uuid ... ok 100 ms
enum ... ok 142 ms
money ... ok 109 ms
rangetypes ... ok 433 ms
pg_lsn ... ok 64 ms
regproc ... ok 91 ms
parallel group (20 tests): lseg path circle time macaddr
create_function_0 timetz line macaddr8 numerology point interval inet
date strings polygon box multirangetypes timestamp timestamptz
strings ... ok 166 ms
numerology ... ok 89 ms
point ... ok 96 ms
lseg ... ok 35 ms
line ... ok 70 ms
box ... ok 255 ms
path ... ok 50 ms
polygon ... ok 237 ms
circle ... ok 53 ms
date ... ok 127 ms
time ... ok 60 ms
timetz ... ok 67 ms
timestamp ... ok 379 ms
timestamptz ... ok 413 ms
interval ... ok 97 ms
inet ... ok 118 ms
macaddr ... ok 60 ms
macaddr8 ... ok 80 ms
multirangetypes ... ok 307 ms
create_function_0 ... ok 63 ms
parallel group (12 tests): comments unicode misc_sanity tstypes xid
expressions horology geometry mvcc type_sanity regex opr_sanity
geometry ... ok 140 ms
horology ... ok 120 ms
tstypes ... ok 53 ms
regex ... ok 335 ms
type_sanity ... ok 155 ms
opr_sanity ... ok 355 ms
misc_sanity ... ok 43 ms
comments ... ok 20 ms
expressions ... ok 100 ms
unicode ... ok 25 ms
xid ... ok 56 ms
mvcc ... ok 146 ms
test create_function_1 ... ok 10 ms
test create_type ... ok 30 ms
test create_table ... ok 333 ms
test create_function_2 ... ok 11 ms
parallel group (5 tests): copydml copyselect insert_conflict insert copy
copy ... ok 336 ms
copyselect ... ok 34 ms
copydml ... ok 28 ms
insert ... ok 291 ms
insert_conflict ... FAILED (test process exited with
exit code 2) 239 ms
parallel group (3 tests): create_operator create_procedure create_misc
create_misc ... ok 131 ms
create_operator ... ok 29 ms
create_procedure ... ok 52 ms
parallel group (5 tests): create_view create_index_spgist
index_including create_index index_including_gist
create_index ... FAILED (test process exited with
exit code 2) 3801 ms
create_index_spgist ... ok 523 ms
create_view ... FAILED (test process exited with
exit code 2) 339 ms
index_including ... FAILED (test process exited with
exit code 2) 3801 ms
index_including_gist ... FAILED (test process exited with
exit code 2) 3801 ms
parallel group (16 tests): create_aggregate create_cast typed_table
drop_if_exists roleattributes create_am hash_func updatable_views errors
infinite_recurse create_function_3 triggers constraints select inherit
vacuum
create_aggregate ... FAILED (test process exited with
exit code 2) 164 ms
create_function_3 ... FAILED (test process exited with
exit code 2) 164 ms
create_cast ... FAILED (test process exited with
exit code 2) 164 ms
constraints ... FAILED (test process exited with
exit code 2) 181 ms
triggers ... FAILED (test process exited with
exit code 2) 181 ms
select ... FAILED (test process exited with
exit code 2) 181 ms
inherit ... FAILED (test process exited with
exit code 2) 181 ms
typed_table ... FAILED (test process exited with
exit code 2) 163 ms
vacuum ... FAILED (test process exited with
exit code 2) 180 ms
drop_if_exists ... FAILED (test process exited with
exit code 2) 163 ms
updatable_views ... FAILED (test process exited with
exit code 2) 163 ms
roleattributes ... FAILED (test process exited with
exit code 2) 163 ms
create_am ... FAILED (test process exited with
exit code 2) 163 ms
hash_func ... FAILED (test process exited with
exit code 2) 162 ms
errors ... FAILED (test process exited with
exit code 2) 162 ms
infinite_recurse ... FAILED (test process exited with
exit code 2) 162 ms
test sanity_check ... FAILED (test process exited with
exit code 2) 26 ms
parallel group (20 tests): select_into subselect select_distinct arrays
join namespace hash_index select_having portals transactions aggregates
random update delete union btree_index select_implicit
select_distinct_on prepared_xacts case
select_into ... FAILED (test process exited with
exit code 2) 20 ms
select_distinct ... FAILED (test process exited with
exit code 2) 21 ms
select_distinct_on ... FAILED (test process exited with
exit code 2) 26 ms
select_implicit ... FAILED (test process exited with
exit code 2) 26 ms
select_having ... FAILED (test process exited with
exit code 2) 23 ms
subselect ... FAILED (test process exited with
exit code 2) 20 ms
union ... FAILED (test process exited with
exit code 2) 25 ms
case ... FAILED (test process exited with
exit code 2) 27 ms
join ... FAILED (test process exited with
exit code 2) 22 ms
aggregates ... FAILED (test process exited with
exit code 2) 24 ms
transactions ... FAILED (test process exited with
exit code 2) 24 ms
random ... failed (ignored) (test process
exited with exit code 2) 24 ms
portals ... FAILED (test process exited with
exit code 2) 23 ms
arrays ... FAILED (test process exited with
exit code 2) 20 ms
btree_index ... FAILED (test process exited with
exit code 2) 25 ms
hash_index ... FAILED (test process exited with
exit code 2) 22 ms
update ... FAILED (test process exited with
exit code 2) 23 ms
delete ... FAILED (test process exited with
exit code 2) 24 ms
namespace ... FAILED (test process exited with
exit code 2) 21 ms
prepared_xacts ... FAILED (test process exited with
exit code 2) 25 ms
parallel group (20 tests): gist brin identity generated password
tablesample lock matview replica_identity rowsecurity security_label
object_address drop_operator groupingsets join_hash privileges collate
init_privs spgist gin
brin ... FAILED (test process exited with
exit code 2) 15 ms
gin ... FAILED (test process exited with
exit code 2) 22 ms
gist ... FAILED (test process exited with
exit code 2) 13 ms
spgist ... FAILED (test process exited with
exit code 2) 22 ms
privileges ... FAILED (test process exited with
exit code 2) 19 ms
init_privs ... FAILED (test process exited with
exit code 2) 21 ms
security_label ... FAILED (test process exited with
exit code 2) 17 ms
collate ... FAILED (test process exited with
exit code 2) 20 ms
matview ... FAILED (test process exited with
exit code 2) 17 ms
lock ... FAILED (test process exited with
exit code 2) 15 ms
replica_identity ... FAILED (test process exited with
exit code 2) 17 ms
rowsecurity ... FAILED (test process exited with
exit code 2) 17 ms
object_address ... FAILED (test process exited with
exit code 2) 17 ms
tablesample ... FAILED (test process exited with
exit code 2) 15 ms
groupingsets ... FAILED (test process exited with
exit code 2) 17 ms
drop_operator ... FAILED (test process exited with
exit code 2) 17 ms
password ... FAILED (test process exited with
exit code 2) 14 ms
identity ... FAILED (test process exited with
exit code 2) 13 ms
generated ... FAILED (test process exited with
exit code 2) 14 ms
join_hash ... FAILED (test process exited with
exit code 2) 18 ms
parallel group (2 tests): brin_multi brin_bloom
brin_bloom ... FAILED (test process exited with
exit code 2) 4 ms
brin_multi ... FAILED (test process exited with
exit code 2) 4 ms
parallel group (14 tests): async create_table_like collate.icu.utf8
misc sysviews alter_operator tidscan tidrangescan alter_generic tsrf
incremental_sort misc_functions tid dbsize
create_table_like ... FAILED (test process exited with
exit code 2) 10 ms
alter_generic ... FAILED (test process exited with
exit code 2) 15 ms
alter_operator ... FAILED (test process exited with
exit code 2) 13 ms
misc ... FAILED (test process exited with
exit code 2) 11 ms
async ... FAILED (test process exited with
exit code 2) 9 ms
dbsize ... FAILED (test process exited with
exit code 2) 17 ms
misc_functions ... FAILED (test process exited with
exit code 2) 14 ms
sysviews ... FAILED (test process exited with
exit code 2) 12 ms
tsrf ... FAILED (test process exited with
exit code 2) 14 ms
tid ... FAILED (test process exited with
exit code 2) 16 ms
tidscan ... FAILED (test process exited with
exit code 2) 13 ms
tidrangescan ... FAILED (test process exited with
exit code 2) 13 ms
collate.icu.utf8 ... FAILED (test process exited with
exit code 2) 9 ms
incremental_sort ... FAILED (test process exited with
exit code 2) 13 ms
parallel group (6 tests): amutils psql_crosstab collate.linux.utf8 psql
stats_ext rules
rules ... FAILED (test process exited with
exit code 2) 8 ms
psql ... FAILED (test process exited with
exit code 2) 7 ms
psql_crosstab ... FAILED (test process exited with
exit code 2) 7 ms
amutils ... FAILED (test process exited with
exit code 2) 7 ms
stats_ext ... FAILED (test process exited with
exit code 2) 8 ms
collate.linux.utf8 ... FAILED (test process exited with
exit code 2) 7 ms
test select_parallel ... FAILED (test process exited with
exit code 2) 3 ms
test write_parallel ... FAILED (test process exited with
exit code 2) 3 ms
parallel group (2 tests): publication subscription
publication ... FAILED (test process exited with
exit code 2) 4 ms
subscription ... FAILED (test process exited with
exit code 2) 4 ms
parallel group (17 tests): select_views foreign_key xmlmap window
functional_deps tsearch cluster combocid bitmapops tsdicts equivclass
guc indirect_toast advisory_lock foreign_data dependency portals_p2
select_views ... FAILED (test process exited with
exit code 2) 11 ms
portals_p2 ... FAILED (test process exited with
exit code 2) 20 ms
foreign_key ... FAILED (test process exited with
exit code 2) 11 ms
cluster ... FAILED (test process exited with
exit code 2) 15 ms
dependency ... FAILED (test process exited with
exit code 2) 19 ms
guc ... FAILED (test process exited with
exit code 2) 16 ms
bitmapops ... FAILED (test process exited with
exit code 2) 16 ms
combocid ... FAILED (test process exited with
exit code 2) 15 ms
tsearch ... FAILED (test process exited with
exit code 2) 14 ms
tsdicts ... FAILED (test process exited with
exit code 2) 16 ms
foreign_data ... FAILED (test process exited with
exit code 2) 17 ms
window ... FAILED (test process exited with
exit code 2) 12 ms
xmlmap ... FAILED (test process exited with
exit code 2) 12 ms
functional_deps ... FAILED (test process exited with
exit code 2) 13 ms
advisory_lock ... FAILED (test process exited with
exit code 2) 16 ms
indirect_toast ... FAILED (test process exited with
exit code 2) 16 ms
equivclass ... FAILED (test process exited with
exit code 2) 15 ms
parallel group (6 tests): json json_encoding jsonb jsonpath_encoding
jsonb_jsonpath jsonpath
json ... FAILED (test process exited with
exit code 2) 4 ms
jsonb ... FAILED (test process exited with
exit code 2) 6 ms
json_encoding ... FAILED (test process exited with
exit code 2) 5 ms
jsonpath ... FAILED (test process exited with
exit code 2) 10 ms
jsonpath_encoding ... FAILED (test process exited with
exit code 2) 6 ms
jsonb_jsonpath ... FAILED (test process exited with
exit code 2) 7 ms
parallel group (19 tests): plpgsql limit rowtypes sequence largeobject
returning domain polymorphism plancache prepare alter_table truncate
temp rangefuncs with copy2 conversion schema_variables xml
plancache ... FAILED (test process exited with
exit code 2) 16 ms
limit ... FAILED (test process exited with
exit code 2) 10 ms
plpgsql ... FAILED (test process exited with
exit code 2) 7 ms
copy2 ... FAILED (test process exited with
exit code 2) 25 ms
temp ... FAILED (test process exited with
exit code 2) 21 ms
domain ... FAILED (test process exited with
exit code 2) 13 ms
rangefuncs ... FAILED (test process exited with
exit code 2) 22 ms
prepare ... FAILED (test process exited with
exit code 2) 19 ms
conversion ... FAILED (test process exited with
exit code 2) 24 ms
truncate ... FAILED (test process exited with
exit code 2) 19 ms
alter_table ... FAILED (test process exited with
exit code 2) 18 ms
sequence ... FAILED (test process exited with
exit code 2) 11 ms
polymorphism ... FAILED (test process exited with
exit code 2) 13 ms
rowtypes ... FAILED (test process exited with
exit code 2) 10 ms
returning ... FAILED (test process exited with
exit code 2) 11 ms
largeobject ... FAILED (test process exited with
exit code 2) 10 ms
with ... FAILED (test process exited with
exit code 2) 22 ms
xml ... FAILED (test process exited with
exit code 2) 23 ms
schema_variables ... FAILED (test process exited with
exit code 2) 23 ms
parallel group (11 tests): explain hash_part partition_info reloptions
memoize compression partition_aggregate partition_join indexing
partition_prune tuplesort
partition_join ... FAILED 902 ms
partition_prune ... ok 1006 ms
reloptions ... ok 106 ms
hash_part ... ok 99 ms
indexing ... ok 929 ms
partition_aggregate ... ok 791 ms
partition_info ... ok 104 ms
tuplesort ... ok 1099 ms
explain ... ok 90 ms
compression ... ok 214 ms
memoize ... ok 109 ms
parallel group (2 tests): event_trigger oidjoins
event_trigger ... ok 107 ms
oidjoins ... ok 157 ms
test fast_default ... ok 138 ms
test stats ... ok 617 ms

On 9/9/21 6:59 AM, Pavel Stehule wrote:
> Hi
>
> fresh rebase
>
> Regards
>
> Pavel
>

Hi

čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> > [schema-variables-20210909.patch]
>
> Hi Pavel,
>
> The patch applies and compiles fine but 'make check' for the
> assert-enabled fails on 131 out of 210 tests.
>

This morning I tested it. I'll recheck it.

Pavel

> (while compiling HEAD checks run without errors for both assert-disabled
> and assert-enabled)
>
>
> Erik Rijkers
>
>
> test tablespace ... ok 303 ms
> parallel group (20 tests): oid char pg_lsn int2 varchar txid int4
> regproc uuid float4 text name money boolean bit float8 int8 enum numeric
> rangetypes
> boolean ... ok 112 ms
> char ... ok 57 ms
> name ... ok 106 ms
> varchar ... ok 74 ms
> text ... ok 106 ms
> int2 ... ok 73 ms
> int4 ... ok 92 ms
> int8 ... ok 130 ms
> oid ... ok 55 ms
> float4 ... ok 102 ms
> float8 ... ok 126 ms
> bit ... ok 124 ms
> numeric ... ok 362 ms
> txid ... ok 87 ms
> uuid ... ok 100 ms
> enum ... ok 142 ms
> money ... ok 109 ms
> rangetypes ... ok 433 ms
> pg_lsn ... ok 64 ms
> regproc ... ok 91 ms
> parallel group (20 tests): lseg path circle time macaddr
> create_function_0 timetz line macaddr8 numerology point interval inet
> date strings polygon box multirangetypes timestamp timestamptz
> strings ... ok 166 ms
> numerology ... ok 89 ms
> point ... ok 96 ms
> lseg ... ok 35 ms
> line ... ok 70 ms
> box ... ok 255 ms
> path ... ok 50 ms
> polygon ... ok 237 ms
> circle ... ok 53 ms
> date ... ok 127 ms
> time ... ok 60 ms
> timetz ... ok 67 ms
> timestamp ... ok 379 ms
> timestamptz ... ok 413 ms
> interval ... ok 97 ms
> inet ... ok 118 ms
> macaddr ... ok 60 ms
> macaddr8 ... ok 80 ms
> multirangetypes ... ok 307 ms
> create_function_0 ... ok 63 ms
> parallel group (12 tests): comments unicode misc_sanity tstypes xid
> expressions horology geometry mvcc type_sanity regex opr_sanity
> geometry ... ok 140 ms
> horology ... ok 120 ms
> tstypes ... ok 53 ms
> regex ... ok 335 ms
> type_sanity ... ok 155 ms
> opr_sanity ... ok 355 ms
> misc_sanity ... ok 43 ms
> comments ... ok 20 ms
> expressions ... ok 100 ms
> unicode ... ok 25 ms
> xid ... ok 56 ms
> mvcc ... ok 146 ms
> test create_function_1 ... ok 10 ms
> test create_type ... ok 30 ms
> test create_table ... ok 333 ms
> test create_function_2 ... ok 11 ms
> parallel group (5 tests): copydml copyselect insert_conflict insert copy
> copy ... ok 336 ms
> copyselect ... ok 34 ms
> copydml ... ok 28 ms
> insert ... ok 291 ms
> insert_conflict ... FAILED (test process exited with
> exit code 2) 239 ms
> parallel group (3 tests): create_operator create_procedure create_misc
> create_misc ... ok 131 ms
> create_operator ... ok 29 ms
> create_procedure ... ok 52 ms
> parallel group (5 tests): create_view create_index_spgist
> index_including create_index index_including_gist
> create_index ... FAILED (test process exited with
> exit code 2) 3801 ms
> create_index_spgist ... ok 523 ms
> create_view ... FAILED (test process exited with
> exit code 2) 339 ms
> index_including ... FAILED (test process exited with
> exit code 2) 3801 ms
> index_including_gist ... FAILED (test process exited with
> exit code 2) 3801 ms
> parallel group (16 tests): create_aggregate create_cast typed_table
> drop_if_exists roleattributes create_am hash_func updatable_views errors
> infinite_recurse create_function_3 triggers constraints select inherit
> vacuum
> create_aggregate ... FAILED (test process exited with
> exit code 2) 164 ms
> create_function_3 ... FAILED (test process exited with
> exit code 2) 164 ms
> create_cast ... FAILED (test process exited with
> exit code 2) 164 ms
> constraints ... FAILED (test process exited with
> exit code 2) 181 ms
> triggers ... FAILED (test process exited with
> exit code 2) 181 ms
> select ... FAILED (test process exited with
> exit code 2) 181 ms
> inherit ... FAILED (test process exited with
> exit code 2) 181 ms
> typed_table ... FAILED (test process exited with
> exit code 2) 163 ms
> vacuum ... FAILED (test process exited with
> exit code 2) 180 ms
> drop_if_exists ... FAILED (test process exited with
> exit code 2) 163 ms
> updatable_views ... FAILED (test process exited with
> exit code 2) 163 ms
> roleattributes ... FAILED (test process exited with
> exit code 2) 163 ms
> create_am ... FAILED (test process exited with
> exit code 2) 163 ms
> hash_func ... FAILED (test process exited with
> exit code 2) 162 ms
> errors ... FAILED (test process exited with
> exit code 2) 162 ms
> infinite_recurse ... FAILED (test process exited with
> exit code 2) 162 ms
> test sanity_check ... FAILED (test process exited with
> exit code 2) 26 ms
> parallel group (20 tests): select_into subselect select_distinct arrays
> join namespace hash_index select_having portals transactions aggregates
> random update delete union btree_index select_implicit
> select_distinct_on prepared_xacts case
> select_into ... FAILED (test process exited with
> exit code 2) 20 ms
> select_distinct ... FAILED (test process exited with
> exit code 2) 21 ms
> select_distinct_on ... FAILED (test process exited with
> exit code 2) 26 ms
> select_implicit ... FAILED (test process exited with
> exit code 2) 26 ms
> select_having ... FAILED (test process exited with
> exit code 2) 23 ms
> subselect ... FAILED (test process exited with
> exit code 2) 20 ms
> union ... FAILED (test process exited with
> exit code 2) 25 ms
> case ... FAILED (test process exited with
> exit code 2) 27 ms
> join ... FAILED (test process exited with
> exit code 2) 22 ms
> aggregates ... FAILED (test process exited with
> exit code 2) 24 ms
> transactions ... FAILED (test process exited with
> exit code 2) 24 ms
> random ... failed (ignored) (test process
> exited with exit code 2) 24 ms
> portals ... FAILED (test process exited with
> exit code 2) 23 ms
> arrays ... FAILED (test process exited with
> exit code 2) 20 ms
> btree_index ... FAILED (test process exited with
> exit code 2) 25 ms
> hash_index ... FAILED (test process exited with
> exit code 2) 22 ms
> update ... FAILED (test process exited with
> exit code 2) 23 ms
> delete ... FAILED (test process exited with
> exit code 2) 24 ms
> namespace ... FAILED (test process exited with
> exit code 2) 21 ms
> prepared_xacts ... FAILED (test process exited with
> exit code 2) 25 ms
> parallel group (20 tests): gist brin identity generated password
> tablesample lock matview replica_identity rowsecurity security_label
> object_address drop_operator groupingsets join_hash privileges collate
> init_privs spgist gin
> brin ... FAILED (test process exited with
> exit code 2) 15 ms
> gin ... FAILED (test process exited with
> exit code 2) 22 ms
> gist ... FAILED (test process exited with
> exit code 2) 13 ms
> spgist ... FAILED (test process exited with
> exit code 2) 22 ms
> privileges ... FAILED (test process exited with
> exit code 2) 19 ms
> init_privs ... FAILED (test process exited with
> exit code 2) 21 ms
> security_label ... FAILED (test process exited with
> exit code 2) 17 ms
> collate ... FAILED (test process exited with
> exit code 2) 20 ms
> matview ... FAILED (test process exited with
> exit code 2) 17 ms
> lock ... FAILED (test process exited with
> exit code 2) 15 ms
> replica_identity ... FAILED (test process exited with
> exit code 2) 17 ms
> rowsecurity ... FAILED (test process exited with
> exit code 2) 17 ms
> object_address ... FAILED (test process exited with
> exit code 2) 17 ms
> tablesample ... FAILED (test process exited with
> exit code 2) 15 ms
> groupingsets ... FAILED (test process exited with
> exit code 2) 17 ms
> drop_operator ... FAILED (test process exited with
> exit code 2) 17 ms
> password ... FAILED (test process exited with
> exit code 2) 14 ms
> identity ... FAILED (test process exited with
> exit code 2) 13 ms
> generated ... FAILED (test process exited with
> exit code 2) 14 ms
> join_hash ... FAILED (test process exited with
> exit code 2) 18 ms
> parallel group (2 tests): brin_multi brin_bloom
> brin_bloom ... FAILED (test process exited with
> exit code 2) 4 ms
> brin_multi ... FAILED (test process exited with
> exit code 2) 4 ms
> parallel group (14 tests): async create_table_like collate.icu.utf8
> misc sysviews alter_operator tidscan tidrangescan alter_generic tsrf
> incremental_sort misc_functions tid dbsize
> create_table_like ... FAILED (test process exited with
> exit code 2) 10 ms
> alter_generic ... FAILED (test process exited with
> exit code 2) 15 ms
> alter_operator ... FAILED (test process exited with
> exit code 2) 13 ms
> misc ... FAILED (test process exited with
> exit code 2) 11 ms
> async ... FAILED (test process exited with
> exit code 2) 9 ms
> dbsize ... FAILED (test process exited with
> exit code 2) 17 ms
> misc_functions ... FAILED (test process exited with
> exit code 2) 14 ms
> sysviews ... FAILED (test process exited with
> exit code 2) 12 ms
> tsrf ... FAILED (test process exited with
> exit code 2) 14 ms
> tid ... FAILED (test process exited with
> exit code 2) 16 ms
> tidscan ... FAILED (test process exited with
> exit code 2) 13 ms
> tidrangescan ... FAILED (test process exited with
> exit code 2) 13 ms
> collate.icu.utf8 ... FAILED (test process exited with
> exit code 2) 9 ms
> incremental_sort ... FAILED (test process exited with
> exit code 2) 13 ms
> parallel group (6 tests): amutils psql_crosstab collate.linux.utf8 psql
> stats_ext rules
> rules ... FAILED (test process exited with
> exit code 2) 8 ms
> psql ... FAILED (test process exited with
> exit code 2) 7 ms
> psql_crosstab ... FAILED (test process exited with
> exit code 2) 7 ms
> amutils ... FAILED (test process exited with
> exit code 2) 7 ms
> stats_ext ... FAILED (test process exited with
> exit code 2) 8 ms
> collate.linux.utf8 ... FAILED (test process exited with
> exit code 2) 7 ms
> test select_parallel ... FAILED (test process exited with
> exit code 2) 3 ms
> test write_parallel ... FAILED (test process exited with
> exit code 2) 3 ms
> parallel group (2 tests): publication subscription
> publication ... FAILED (test process exited with
> exit code 2) 4 ms
> subscription ... FAILED (test process exited with
> exit code 2) 4 ms
> parallel group (17 tests): select_views foreign_key xmlmap window
> functional_deps tsearch cluster combocid bitmapops tsdicts equivclass
> guc indirect_toast advisory_lock foreign_data dependency portals_p2
> select_views ... FAILED (test process exited with
> exit code 2) 11 ms
> portals_p2 ... FAILED (test process exited with
> exit code 2) 20 ms
> foreign_key ... FAILED (test process exited with
> exit code 2) 11 ms
> cluster ... FAILED (test process exited with
> exit code 2) 15 ms
> dependency ... FAILED (test process exited with
> exit code 2) 19 ms
> guc ... FAILED (test process exited with
> exit code 2) 16 ms
> bitmapops ... FAILED (test process exited with
> exit code 2) 16 ms
> combocid ... FAILED (test process exited with
> exit code 2) 15 ms
> tsearch ... FAILED (test process exited with
> exit code 2) 14 ms
> tsdicts ... FAILED (test process exited with
> exit code 2) 16 ms
> foreign_data ... FAILED (test process exited with
> exit code 2) 17 ms
> window ... FAILED (test process exited with
> exit code 2) 12 ms
> xmlmap ... FAILED (test process exited with
> exit code 2) 12 ms
> functional_deps ... FAILED (test process exited with
> exit code 2) 13 ms
> advisory_lock ... FAILED (test process exited with
> exit code 2) 16 ms
> indirect_toast ... FAILED (test process exited with
> exit code 2) 16 ms
> equivclass ... FAILED (test process exited with
> exit code 2) 15 ms
> parallel group (6 tests): json json_encoding jsonb jsonpath_encoding
> jsonb_jsonpath jsonpath
> json ... FAILED (test process exited with
> exit code 2) 4 ms
> jsonb ... FAILED (test process exited with
> exit code 2) 6 ms
> json_encoding ... FAILED (test process exited with
> exit code 2) 5 ms
> jsonpath ... FAILED (test process exited with
> exit code 2) 10 ms
> jsonpath_encoding ... FAILED (test process exited with
> exit code 2) 6 ms
> jsonb_jsonpath ... FAILED (test process exited with
> exit code 2) 7 ms
> parallel group (19 tests): plpgsql limit rowtypes sequence largeobject
> returning domain polymorphism plancache prepare alter_table truncate
> temp rangefuncs with copy2 conversion schema_variables xml
> plancache ... FAILED (test process exited with
> exit code 2) 16 ms
> limit ... FAILED (test process exited with
> exit code 2) 10 ms
> plpgsql ... FAILED (test process exited with
> exit code 2) 7 ms
> copy2 ... FAILED (test process exited with
> exit code 2) 25 ms
> temp ... FAILED (test process exited with
> exit code 2) 21 ms
> domain ... FAILED (test process exited with
> exit code 2) 13 ms
> rangefuncs ... FAILED (test process exited with
> exit code 2) 22 ms
> prepare ... FAILED (test process exited with
> exit code 2) 19 ms
> conversion ... FAILED (test process exited with
> exit code 2) 24 ms
> truncate ... FAILED (test process exited with
> exit code 2) 19 ms
> alter_table ... FAILED (test process exited with
> exit code 2) 18 ms
> sequence ... FAILED (test process exited with
> exit code 2) 11 ms
> polymorphism ... FAILED (test process exited with
> exit code 2) 13 ms
> rowtypes ... FAILED (test process exited with
> exit code 2) 10 ms
> returning ... FAILED (test process exited with
> exit code 2) 11 ms
> largeobject ... FAILED (test process exited with
> exit code 2) 10 ms
> with ... FAILED (test process exited with
> exit code 2) 22 ms
> xml ... FAILED (test process exited with
> exit code 2) 23 ms
> schema_variables ... FAILED (test process exited with
> exit code 2) 23 ms
> parallel group (11 tests): explain hash_part partition_info reloptions
> memoize compression partition_aggregate partition_join indexing
> partition_prune tuplesort
> partition_join ... FAILED 902 ms
> partition_prune ... ok 1006 ms
> reloptions ... ok 106 ms
> hash_part ... ok 99 ms
> indexing ... ok 929 ms
> partition_aggregate ... ok 791 ms
> partition_info ... ok 104 ms
> tuplesort ... ok 1099 ms
> explain ... ok 90 ms
> compression ... ok 214 ms
> memoize ... ok 109 ms
> parallel group (2 tests): event_trigger oidjoins
> event_trigger ... ok 107 ms
> oidjoins ... ok 157 ms
> test fast_default ... ok 138 ms
> test stats ... ok 617 ms
>
>
>
> On 9/9/21 6:59 AM, Pavel Stehule wrote:
> > Hi
> >
> > fresh rebase
> >
> > Regards
> >
> > Pavel
> >
>

Le 09/09/2021 à 11:40, Pavel Stehule a écrit :
> Hi
>
> čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl
> <mailto:er(at)xs4all(dot)nl>> napsal:
>
>  > [schema-variables-20210909.patch]
>
> Hi Pavel,
>
> The patch applies and compiles fine but 'make check' for the
> assert-enabled fails on 131 out of 210 tests.
>
>
> This morning I tested it. I'll recheck it.
>
> Pavel
>

I had not this problem yesterday.

--
Gilles Darold
http://www.darold.net/

čt 9. 9. 2021 v 13:17 odesílatel Gilles Darold <gilles(at)darold(dot)net> napsal:

> Le 09/09/2021 à 11:40, Pavel Stehule a écrit :
>
> Hi
>
> čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:
>
>> > [schema-variables-20210909.patch]
>>
>> Hi Pavel,
>>
>> The patch applies and compiles fine but 'make check' for the
>> assert-enabled fails on 131 out of 210 tests.
>>
>
> This morning I tested it. I'll recheck it.
>
> Pavel
>
>
> I had not this problem yesterday.
>

I am able to reproduce it. Looks like some current changes of Nodes don't
work with this patch. I have to investigate it.

Regards

Pavel

> --
> Gilles Daroldhttp://www.darold.net/
>
>

Hi

čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> > [schema-variables-20210909.patch]
>
> Hi Pavel,
>
> The patch applies and compiles fine but 'make check' for the
> assert-enabled fails on 131 out of 210 tests.
>
> (while compiling HEAD checks run without errors for both assert-disabled
> and assert-enabled)
>
>

Please, check, attached patch. I fixed a routine for processing a list of
identifiers - now it works with the identifier's node more sensitive.
Previous implementation of strVal was more tolerant.

Regards

Pavel

>

On 9/10/21 10:06 AM, Pavel Stehule wrote:
> Hi
>
> čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:
>
>>
>> Hi Pavel,
>>
>> The patch applies and compiles fine but 'make check' for the
>> assert-enabled fails on 131 out of 210 tests.
>>
>> (while compiling HEAD checks run without errors for both assert-disabled
>> and assert-enabled)
>>
>>
>
> Please, check, attached patch. I fixed a routine for processing a list of
> identifiers - now it works with the identifier's node more sensitive.
> Previous implementation of strVal was more tolerant.

> [schema-variables-20210910.patch]

Apply, compile, make, & check(-world), and my small testsuite OK.

So all's well again - Ready for committer!

Thanks,

Erik Rijkers

> Regards
>
> Pavel
>
>
>>
>

pá 10. 9. 2021 v 10:32 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> On 9/10/21 10:06 AM, Pavel Stehule wrote:
> > Hi
> >
> > čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:
> >
> >>
> >> Hi Pavel,
> >>
> >> The patch applies and compiles fine but 'make check' for the
> >> assert-enabled fails on 131 out of 210 tests.
> >>
> >> (while compiling HEAD checks run without errors for both assert-disabled
> >> and assert-enabled)
> >>
> >>
> >
> > Please, check, attached patch. I fixed a routine for processing a list of
> > identifiers - now it works with the identifier's node more sensitive.
> > Previous implementation of strVal was more tolerant.
>
> > [schema-variables-20210910.patch]
>
> Apply, compile, make, & check(-world), and my small testsuite OK.
>
> So all's well again - Ready for committer!
>

Thank you for check and for report

Regards

Pavel

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

On Fri, Sep 10, 2021 at 10:06:04AM +0200, Pavel Stehule wrote:
> Hi
>
> čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:
>
> > > [schema-variables-20210909.patch]
> >
> > Hi Pavel,
> >
> > The patch applies and compiles fine but 'make check' for the
> > assert-enabled fails on 131 out of 210 tests.
> >
> > (while compiling HEAD checks run without errors for both assert-disabled
> > and assert-enabled)
> >
> >
>
> Please, check, attached patch. I fixed a routine for processing a list of
> identifiers - now it works with the identifier's node more sensitive.
> Previous implementation of strVal was more tolerant.
>

Hi Pavel,

Just noted that there is no support for REASSIGN OWNED BY:

"""
regression=# create variable random_number numeric;
CREATE VARIABLE
regression=# alter variable random_number owner to jcm;
ALTER VARIABLE
regression=# reassign owned by jcm to jaime;
ERROR: unexpected classid 9222
"""

TEMP variables are not schema variables? at least not attached to the
schema one expects:

"""
regression=# create temp variable random_number numeric ;
CREATE VARIABLE
regression=# \dV
List of variables
Schema | Name | Type | Is nullable | Is mutable | Default | Owner | Transaction
al end action
-----------+---------------+---------+-------------+------------+---------+----------+------------
--------------
pg_temp_4 | random_number | numeric | t | t | | jcasanov |
(1 row)

regression=# select public.random_number;
ERROR: missing FROM-clause entry for table "public"
LINE 1: select public.random_number;
^
"""

There was a comment that TEMP variables should be DECLAREd instead of
CREATEd, i guess that is because those have similar behaviour. At least,
I would like to see similar messages when using the ON COMMIT DROP
option in a TEMP variable:

"""
regression=# create temp variable random_number numeric on commit drop;
CREATE VARIABLE
regression=# \dV
Did not find any schema variables.
regression=# declare q cursor for select 1;
ERROR: DECLARE CURSOR can only be used in transaction blocks
"""

About that, why are you not using syntax ON COMMIT RESET instead on
inventing ON TRANSACTION END RESET? seems better because you already use
ON COMMIT DROP.

I will test more this patch tomorrow. Great work, very complete.

--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Hi

> Just noted that there is no support for REASSIGN OWNED BY:
>
> """
> regression=# create variable random_number numeric;
> CREATE VARIABLE
> regression=# alter variable random_number owner to jcm;
> ALTER VARIABLE
> regression=# reassign owned by jcm to jaime;
> ERROR: unexpected classid 9222
> """
>
>
should be fixed by the attached patch, please check.

> TEMP variables are not schema variables? at least not attached to the
> schema one expects:
>

temp variables are schema variables like any other. But they are created in
temp schema - like temp tables.
I designed it in consistency with temporary tables.

> """
> regression=# create temp variable random_number numeric ;
> CREATE VARIABLE
> regression=# \dV
> List of variables
> Schema | Name | Type | Is nullable | Is mutable | Default
> | Owner | Transaction
> al end action
>
> -----------+---------------+---------+-------------+------------+---------+----------+------------
> --------------
> pg_temp_4 | random_number | numeric | t | t |
> | jcasanov |
> (1 row)
>
> regression=# select public.random_number;
> ERROR: missing FROM-clause entry for table "public"
> LINE 1: select public.random_number;
> ^
> """
>
> There was a comment that TEMP variables should be DECLAREd instead of
> CREATEd, i guess that is because those have similar behaviour. At least,
> I would like to see similar messages when using the ON COMMIT DROP
> option in a TEMP variable:
>

I don't remember this comment. When I talked about similarity with the
DECLARE statement, I thought about semantic similarity with T-SQL
(Microsoft SQL) DECLARE command. Unfortunately, DECLARE command is pretty
messy - it exists in SQL, it exists in SQL/PSM and it exists in T-SQL - and
every time has similar syntax, but partially different semantics. For me -
CREATE TEMP VARIABLE creates session's life limited variable (by default),
similarly like DECLARE @localvariable command from T-SQL.

> """
> regression=# create temp variable random_number numeric on commit drop;
> CREATE VARIABLE
> regression=# \dV
> Did not find any schema variables.
> regression=# declare q cursor for select 1;
> ERROR: DECLARE CURSOR can only be used in transaction blocks
> """
>

I have different result

postgres=# create temp variable random_number numeric on commit drop;
CREATE VARIABLE
postgres=# \dV
List of variables
┌────────┬───────────────┬─────────┬─────────────┬────────────┬─────────┬───────┬──────────────────────────┐
│ Schema │ Name │ Type │ Is nullable │ Is mutable │ Default │
Owner │ Transactional end action │
╞════════╪═══════════════╪═════════╪═════════════╪════════════╪═════════╪═══════╪══════════════════════════╡
│ public │ random_number │ numeric │ t │ t │ │
tom2 │ │
└────────┴───────────────┴─────────┴─────────────┴────────────┴─────────┴───────┴──────────────────────────┘
(1 row)

> About that, why are you not using syntax ON COMMIT RESET instead on
> inventing ON TRANSACTION END RESET? seems better because you already use
> ON COMMIT DROP.
>

I thought about this question for a very long time, and I think the
introduction of a new clause is better, and I will try to explain why.

One part of this patch are DDL statements - and all DDL statements are
consistent with other DDL statements in Postgres. Schema variables DDL
commands are transactional and for TEMP variables we can specify a scope -
session or transaction, and then clause ON COMMIT DROP is used. You should
not need to specify ON ROLLBACK action, because in this case an removing
from system catalogue is only one possible action.

Second part of this patch is holding some value in schema variables or
initialization with default expression. The default behaviour is not
transactional, and the value is stored all session's time by default. But I
think it can be very useful to enforce initialization in some specific
times - now only the end of the transaction is possible to specify. In the
future there can be transaction end, transaction start, rollback, commit,
top query start, top query end, ... This logic is different from the logic
of DDL commands. For DDL commands I need to specify behaviour just for the
COMMIT end. But for reset of non-transactional schema variables I need to
specify any possible end of transaction - COMMIT, ROLLBACK or COMMIT or
ROLLBACK. In this initial version I implemented "ON COMMIT OR ROLLBACK
RESET", and although it is clean I think it is more readable is the clause
that I invented "ON TRANSACTION END". "ON COMMIT RESET" is not exact. "ON
COMMIT OR ROLLBACK RESET" sounds a little bit strange for me, but we use
something similar in trigger definition "ON INSERT OR UPDATE OR DELETE ..."
My opinion is not too strong if "ON TRANSACTION END RESET" or "ON COMMIT
OR ROLLBACK RESET" is better, and I can change it if people will have
different preferences, but I am sure so "ON COMMIT RESET" is not correct in
implemented case. And from the perspective of a PLpgSQL developer, I would
have initialized the variable on any transaction start, so I need to reset
it on any end.

Regards

Pavel

> I will test more this patch tomorrow. Great work, very complete.
>
> --
> Jaime Casanova
> Director de Servicios Profesionales
> SystemGuards - Consultores de PostgreSQL
>

ne 12. 9. 2021 v 17:38 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi
>
>
>
>> Just noted that there is no support for REASSIGN OWNED BY:
>>
>> """
>> regression=# create variable random_number numeric;
>> CREATE VARIABLE
>> regression=# alter variable random_number owner to jcm;
>> ALTER VARIABLE
>> regression=# reassign owned by jcm to jaime;
>> ERROR: unexpected classid 9222
>> """
>>
>>
> should be fixed by the attached patch, please check.
>
>
>
>> TEMP variables are not schema variables? at least not attached to the
>> schema one expects:
>>
>
> temp variables are schema variables like any other. But they are created
> in temp schema - like temp tables.
> I designed it in consistency with temporary tables.
>
>
>> """
>> regression=# create temp variable random_number numeric ;
>> CREATE VARIABLE
>> regression=# \dV
>> List of variables
>> Schema | Name | Type | Is nullable | Is mutable | Default
>> | Owner | Transaction
>> al end action
>>
>> -----------+---------------+---------+-------------+------------+---------+----------+------------
>> --------------
>> pg_temp_4 | random_number | numeric | t | t |
>> | jcasanov |
>> (1 row)
>>
>> regression=# select public.random_number;
>> ERROR: missing FROM-clause entry for table "public"
>> LINE 1: select public.random_number;
>> ^
>> """
>>
>> There was a comment that TEMP variables should be DECLAREd instead of
>> CREATEd, i guess that is because those have similar behaviour. At least,
>> I would like to see similar messages when using the ON COMMIT DROP
>> option in a TEMP variable:
>>
>
> I don't remember this comment. When I talked about similarity with the
> DECLARE statement, I thought about semantic similarity with T-SQL
> (Microsoft SQL) DECLARE command. Unfortunately, DECLARE command is pretty
> messy - it exists in SQL, it exists in SQL/PSM and it exists in T-SQL - and
> every time has similar syntax, but partially different semantics. For me -
> CREATE TEMP VARIABLE creates session's life limited variable (by default),
> similarly like DECLARE @localvariable command from T-SQL.
>

any value of a schema variable has a session (or transaction) life cycle.
But the schema variable itself is persistent. temp schema variable is an
exception. It is limited by session (and the value stored in the variable
is limited to session too).

>
>> """
>> regression=# create temp variable random_number numeric on commit drop;
>> CREATE VARIABLE
>> regression=# \dV
>> Did not find any schema variables.
>> regression=# declare q cursor for select 1;
>> ERROR: DECLARE CURSOR can only be used in transaction blocks
>> """
>>
>
> I have different result
>
> postgres=# create temp variable random_number numeric on commit drop;
> CREATE VARIABLE
> postgres=# \dV
> List of variables
>
> ┌────────┬───────────────┬─────────┬─────────────┬────────────┬─────────┬───────┬──────────────────────────┐
> │ Schema │ Name │ Type │ Is nullable │ Is mutable │ Default │
> Owner │ Transactional end action │
>
> ╞════════╪═══════════════╪═════════╪═════════════╪════════════╪═════════╪═══════╪══════════════════════════╡
> │ public │ random_number │ numeric │ t │ t │ │
> tom2 │ │
>
> └────────┴───────────────┴─────────┴─────────────┴────────────┴─────────┴───────┴──────────────────────────┘
> (1 row)
>
>
>
>> About that, why are you not using syntax ON COMMIT RESET instead on
>> inventing ON TRANSACTION END RESET? seems better because you already use
>> ON COMMIT DROP.
>>
>
> I thought about this question for a very long time, and I think the
> introduction of a new clause is better, and I will try to explain why.
>
> One part of this patch are DDL statements - and all DDL statements are
> consistent with other DDL statements in Postgres. Schema variables DDL
> commands are transactional and for TEMP variables we can specify a scope -
> session or transaction, and then clause ON COMMIT DROP is used. You should
> not need to specify ON ROLLBACK action, because in this case an removing
> from system catalogue is only one possible action.
>
> Second part of this patch is holding some value in schema variables or
> initialization with default expression. The default behaviour is not
> transactional, and the value is stored all session's time by default. But I
> think it can be very useful to enforce initialization in some specific
> times - now only the end of the transaction is possible to specify. In the
> future there can be transaction end, transaction start, rollback, commit,
> top query start, top query end, ... This logic is different from the logic
> of DDL commands. For DDL commands I need to specify behaviour just for the
> COMMIT end. But for reset of non-transactional schema variables I need to
> specify any possible end of transaction - COMMIT, ROLLBACK or COMMIT or
> ROLLBACK. In this initial version I implemented "ON COMMIT OR ROLLBACK
> RESET", and although it is clean I think it is more readable is the clause
> that I invented "ON TRANSACTION END". "ON COMMIT RESET" is not exact. "ON
> COMMIT OR ROLLBACK RESET" sounds a little bit strange for me, but we use
> something similar in trigger definition "ON INSERT OR UPDATE OR DELETE ..."
> My opinion is not too strong if "ON TRANSACTION END RESET" or "ON COMMIT
> OR ROLLBACK RESET" is better, and I can change it if people will have
> different preferences, but I am sure so "ON COMMIT RESET" is not correct in
> implemented case. And from the perspective of a PLpgSQL developer, I would
> have initialized the variable on any transaction start, so I need to reset
> it on any end.
>
> Regards
>
> Pavel
>
>
>
>> I will test more this patch tomorrow. Great work, very complete.
>>
>> --
>> Jaime Casanova
>> Director de Servicios Profesionales
>> SystemGuards - Consultores de PostgreSQL
>>
>

On Sun, Sep 12, 2021 at 05:38:42PM +0200, Pavel Stehule wrote:
> Hi
>
> > """
> > regression=# create temp variable random_number numeric on commit drop;
> > CREATE VARIABLE
> > regression=# \dV
> > Did not find any schema variables.
> > regression=# declare q cursor for select 1;
> > ERROR: DECLARE CURSOR can only be used in transaction blocks
> > """
> >
>
> I have different result
>
> postgres=# create temp variable random_number numeric on commit drop;
> CREATE VARIABLE
> postgres=# \dV
> List of variables
> ┌────────┬───────────────┬─────────┬─────────────┬────────────┬─────────┬───────┬──────────────────────────┐
> │ Schema │ Name │ Type │ Is nullable │ Is mutable │ Default │
> Owner │ Transactional end action │
> ╞════════╪═══════════════╪═════════╪═════════════╪════════════╪═════════╪═══════╪══════════════════════════╡
> │ public │ random_number │ numeric │ t │ t │ │
> tom2 │ │
> └────────┴───────────────┴─────────┴─────────────┴────────────┴─────────┴───────┴──────────────────────────┘
> (1 row)
>
>
>

Hi,

Thanks, will test rebased version.
BTW, that is not the temp variable. You can note it because of the
schema or the lack of a "Transaction end action". That is a normal
non-temp variable that has been created before. A TEMP variable with an
ON COMMIT DROP created outside an explicit transaction will disappear
immediatly like cursor does in the same situation.

--
Jaime Casanova
Director de Servicios Profesionales
SystemGuards - Consultores de PostgreSQL

Hi,
>
> Thanks, will test rebased version.
> BTW, that is not the temp variable. You can note it because of the
> schema or the lack of a "Transaction end action". That is a normal
> non-temp variable that has been created before. A TEMP variable with an
> ON COMMIT DROP created outside an explicit transaction will disappear
> immediatly like cursor does in the same situation.
>

Unfortunately, I don't see it - or I don't understand to your example from
morning mail well

"""
regression=# create temp variable random_number numeric ;
CREATE VARIABLE
regression=# \dV
List of variables
Schema | Name | Type | Is nullable | Is mutable | Default
| Owner | Transaction
al end action
-----------+---------------+---------+-------------+------------+---------+----------+------------
--------------
pg_temp_4 | random_number | numeric | t | t | |
jcasanov |
(1 row)

regression=# select public.random_number;
ERROR: missing FROM-clause entry for table "public"
LINE 1: select public.random_number;
^
"""

>
>
> --
> Jaime Casanova
> Director de Servicios Profesionales
> SystemGuards - Consultores de PostgreSQL
>

Hi

just rebase of patch

Regards

Pavel

Hi

only rebase

Regards

Pavel

čt 16. 9. 2021 v 7:15 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

> Hi
>
> just rebase of patch
>
> Regards
>
> Pavel
>
>

Hi,

I took a quick look at the latest patch version. In general the patch
looks pretty complete and clean, and for now I have only some basic
comments. The attached patch tweaks some of this, along with a couple
additional minor changes that I'll not discuss here.

1) Not sure why we need to call this "schema variables". Most objects
are placed in a schema, and we don't say "schema tables" for example.
And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
inconsistent.

The docs actually use "Global variables" in one place for some reason.

2) I find this a bit confusing:

SELECT non_existent_variable;
test=# select s;
ERROR: column "non_existent_variable" does not exist
LINE 1: select non_existent_variable;

I wonder if this means using SELECT to read variables is a bad idea, and
we should have a separate command, just like we have LET (instead of
just using UPDATE in some way).

3) I've reworded / tweaked a couple places in the docs, but this really
needs a native speaker - I don't have a very good "feeling" for this
technical language so it's probably still quite cumbersome.

4) Is sequential scan of the hash table in clean_cache_callback() a
good idea? I wonder how fast (with how many variables) it'll become
noticeable, but it may be good enough for now and we can add something
better (tracing which variables need resetting) later.

5) In what situation would we call clean_cache_callback() without a
transaction state? If that happens it seems more like a bug, so
maybeelog(ERROR) or Assert() would be more appropriate?

6) free_schema_variable does not actually use the force parameter

7) The target_exprkind expression in transformSelectStmt really needs
some explanation. Because that's chance you'll look at this in 6 months
and understand what it does?

target_exprkind =
(pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
pstate->parentParseState != NULL) ?
EXPR_KIND_SELECT_TARGET : EXPR_KIND_LET_TARGET;

8) immutable variables without a default value

IMO this case should not be allowed. On 2021/08/29 you wrote:

I thought about this case, and I have one scenario, where this
behaviour can be useful. When the variable is declared as IMMUTABLE
NOT NULL without not null default, then any access to the content of
the variable has to fail. I think it can be used for detection,
where and when the variable is first used. So this behavior is
allowed just because I think, so this feature can be interesting for
debugging. If this idea is too strange, I have no problem to disable
this case.

This seems like a really strange use case. In a production code you'll
not do this, because then the variable is useless and the code does not
work at all (it'll just fail whenever it attempts to access the var).
And if you can modify the code, there are other / better ways to do this
(raising an exception, ...).

So this seems pretty useless to me, +1 to disabling it.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

FWIW the patch was marked as RFC for about a year, but there was plenty
of discussion / changes since then, so that seemed premature. I've
switched it back to WoA.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Nov 03, 2021 at 02:05:02PM +0100, Tomas Vondra wrote:
> 3) I've reworded / tweaked a couple places in the docs, but this really
> needs a native speaker - I don't have a very good "feeling" for this
> technical language so it's probably still quite cumbersome.

On Daniel's suggestion, I have reviewed the docs, and then proofread the rest
of the patch. My amendments are in 0003.

--
Justin

Hi

st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)com> napsal:

> Hi,
>
> I took a quick look at the latest patch version. In general the patch
> looks pretty complete and clean, and for now I have only some basic
> comments. The attached patch tweaks some of this, along with a couple
> additional minor changes that I'll not discuss here.
>
>
> 1) Not sure why we need to call this "schema variables". Most objects
> are placed in a schema, and we don't say "schema tables" for example.
> And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
> inconsistent.
>

Yes, there is inconsistency, but I think it is necessary. The name
"variable" is too generic. Theoretically we can use other adjectives like
session variables or global variables and the name will be valid. But it
doesn't describe the fundamentals of design. This is similar to the
package's variables from PL/SQL. These variables are global, session's
variables too. But the usual name is "package variables". So schema
variables are assigned to schemes, and I think a good name can be "schema
variables". But it is not necessary to repeat keyword schema in the CREATE
COMMAND.

My opinion is not too strong in this case, and I can accept just
"variables" or "session's variables" or "global variables", but I am not
sure if these names describe this feature well, because still they are too
generic. There are too many different implementations of session global
variables (see PL/SQL or T-SQL or DB2).

> The docs actually use "Global variables" in one place for some reason.
>
>
> 2) I find this a bit confusing:
>
> SELECT non_existent_variable;
> test=# select s;
> ERROR: column "non_existent_variable" does not exist
> LINE 1: select non_existent_variable;
>
> I wonder if this means using SELECT to read variables is a bad idea, and
> we should have a separate command, just like we have LET (instead of
> just using UPDATE in some way).
>

I am sure so I want to use variables in SELECTs. One interesting case is
using variables in RLS.

I prefer to fix this error message to "column or variable ... does not
exist"

>
>
> 3) I've reworded / tweaked a couple places in the docs, but this really
> needs a native speaker - I don't have a very good "feeling" for this
> technical language so it's probably still quite cumbersome.
>
>
> 4) Is sequential scan of the hash table in clean_cache_callback() a
> good idea? I wonder how fast (with how many variables) it'll become
> noticeable, but it may be good enough for now and we can add something
> better (tracing which variables need resetting) later.
>
>
I have to check it.

>
> 5) In what situation would we call clean_cache_callback() without a
> transaction state? If that happens it seems more like a bug, so
> maybeelog(ERROR) or Assert() would be more appropriate?
>

>
>
> 6) free_schema_variable does not actually use the force parameter
>
>
> 7) The target_exprkind expression in transformSelectStmt really needs
> some explanation. Because that's chance you'll look at this in 6 months
> and understand what it does?
>
> target_exprkind =
> (pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
> pstate->parentParseState != NULL) ?
> EXPR_KIND_SELECT_TARGET : EXPR_KIND_LET_TARGET;
>
>
> 8) immutable variables without a default value
>
> IMO this case should not be allowed. On 2021/08/29 you wrote:
>
> I thought about this case, and I have one scenario, where this
> behaviour can be useful. When the variable is declared as IMMUTABLE
> NOT NULL without not null default, then any access to the content of
> the variable has to fail. I think it can be used for detection,
> where and when the variable is first used. So this behavior is
> allowed just because I think, so this feature can be interesting for
> debugging. If this idea is too strange, I have no problem to disable
> this case.
>
> This seems like a really strange use case. In a production code you'll
> not do this, because then the variable is useless and the code does not
> work at all (it'll just fail whenever it attempts to access the var).
> And if you can modify the code, there are other / better ways to do this
> (raising an exception, ...).
>
> So this seems pretty useless to me, +1 to disabling it.
>

I'll disable it.

>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Sat, Nov 06, 2021 at 04:45:19AM +0100, Pavel Stehule wrote:
> st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> napsal:
> > 1) Not sure why we need to call this "schema variables". Most objects
> > are placed in a schema, and we don't say "schema tables" for example.
> > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
> > inconsistent.

+1

At least the error messages need to be consistent.
It doesn't make sense to have both of these:

+ elog(ERROR, "cache lookup failed for schema variable %u", varid);
+ elog(ERROR, "cache lookup failed for variable %u", varid);

> Yes, there is inconsistency, but I think it is necessary. The name
> "variable" is too generic. Theoretically we can use other adjectives like
> session variables or global variables and the name will be valid. But it
> doesn't describe the fundamentals of design. This is similar to the
> package's variables from PL/SQL. These variables are global, session's
> variables too. But the usual name is "package variables". So schema
> variables are assigned to schemes, and I think a good name can be "schema
> variables". But it is not necessary to repeat keyword schema in the CREATE
> COMMAND.
>
> My opinion is not too strong in this case, and I can accept just
> "variables" or "session's variables" or "global variables", but I am not
> sure if these names describe this feature well, because still they are too
> generic. There are too many different implementations of session global
> variables (see PL/SQL or T-SQL or DB2).

I would prefer "session variable".

To me, this feature seems similar to a CTE (which exists for a single
statement), or a temporary table (which exists for a single transaction). So
"session" conveys a lot more of its meaning than "schema".

But don't rename everything just for me...

--
Justin

so 6. 11. 2021 v 15:57 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Sat, Nov 06, 2021 at 04:45:19AM +0100, Pavel Stehule wrote:
> > st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra <
> tomas(dot)vondra(at)enterprisedb(dot)com> napsal:
> > > 1) Not sure why we need to call this "schema variables". Most objects
> > > are placed in a schema, and we don't say "schema tables" for example.
> > > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
> > > inconsistent.
>
> +1
>
> At least the error messages need to be consistent.
> It doesn't make sense to have both of these:
>
> + elog(ERROR, "cache lookup failed for schema variable %u",
> varid);
> + elog(ERROR, "cache lookup failed for variable %u", varid);
>
> > Yes, there is inconsistency, but I think it is necessary. The name
> > "variable" is too generic. Theoretically we can use other adjectives like
> > session variables or global variables and the name will be valid. But it
> > doesn't describe the fundamentals of design. This is similar to the
> > package's variables from PL/SQL. These variables are global, session's
> > variables too. But the usual name is "package variables". So schema
> > variables are assigned to schemes, and I think a good name can be "schema
> > variables". But it is not necessary to repeat keyword schema in the
> CREATE
> > COMMAND.
> >
> > My opinion is not too strong in this case, and I can accept just
> > "variables" or "session's variables" or "global variables", but I am not
> > sure if these names describe this feature well, because still they are
> too
> > generic. There are too many different implementations of session global
> > variables (see PL/SQL or T-SQL or DB2).
>
> I would prefer "session variable".
>
> To me, this feature seems similar to a CTE (which exists for a single
> statement), or a temporary table (which exists for a single transaction).
> So
> "session" conveys a lot more of its meaning than "schema".
>

It depends on where you are looking. There are two perspectives - data and
metadata. And if I use data perspective, then it is session related. If I
use metadata perspective, then it can be persistent or temporal like
tables. I see strong similarity with Global Temporary Tables - but I think
naming "local temporary tables" and "global temporary tables" can be not
intuitive or messy for a lot of people too. Anyway, if people will try to
find this feature on Google, then probably use keywords "session
variables", so maybe my preference of more technical terminology is obscure
and not practical, and the name "session variables" can be more practical
for other people. If I use the system used for GTT - then the exact name
can be "Global Session Variable". Can we use this name? Or shortly just
Session Variables because we don't support local session variables now.

What do you think about it?

Regards

Pavel

> But don't rename everything just for me...
>
> --
> Justin
>

On 11/6/21 16:40, Pavel Stehule wrote:
>
>
> so 6. 11. 2021 v 15:57 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com
> <mailto:pryzby(at)telsasoft(dot)com>> napsal:
>
> On Sat, Nov 06, 2021 at 04:45:19AM +0100, Pavel Stehule wrote:
> > st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com
> <mailto:tomas(dot)vondra(at)enterprisedb(dot)com>> napsal:
> > > 1) Not sure why we need to call this "schema variables". Most
> objects
> > > are placed in a schema, and we don't say "schema tables" for
> example.
> > > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so
> it's a bit
> > > inconsistent.
>
> +1
>
> At least the error messages need to be consistent.
> It doesn't make sense to have both of these:
>
> +               elog(ERROR, "cache lookup failed for schema variable
> %u", varid);
> +               elog(ERROR, "cache lookup failed for variable %u",
> varid);
>
> > Yes, there is inconsistency, but I think it is necessary. The name
> > "variable" is too generic. Theoretically we can use other
> adjectives like
> > session variables or global variables and the name will be valid.
> But it
> > doesn't describe the fundamentals of design. This is similar to the
> > package's variables from PL/SQL. These variables are global,
> session's
> > variables too. But the usual name is "package variables". So schema
> > variables are assigned to schemes, and I think a good name can be
> "schema
> > variables". But it is not necessary to repeat keyword schema in
> the CREATE
> > COMMAND.
> >
> > My opinion is not too strong in this case, and I can accept just
> > "variables" or "session's variables" or "global variables", but I
> am not
> > sure if these names describe this feature well, because still
> they are too
> > generic. There are too many different implementations of session
> global
> > variables (see PL/SQL or T-SQL or DB2).
>
> I would prefer "session variable".
>
> To me, this feature seems similar to a CTE (which exists for a single
> statement), or a temporary table (which exists for a single
> transaction).  So
> "session" conveys a lot more of its meaning than "schema".
>
>
> It depends on where you are looking. There are two perspectives - data
> and metadata. And if I use data perspective, then it is session related.
> If I use metadata perspective, then it can be persistent or temporal
> like tables.

I think you mean "temporary" not "temporal". This really confused me for
a while, because temporal means "involving time" (e.g. a table with
from/to timestamp range, etc).

> I see strong similarity with Global Temporary Tables - but
> I think naming "local temporary tables" and "global temporary tables"
> can be not intuitive or messy for a lot of people too.

Right, it's a bit like global temporary tables, in the sense that
there's a shared definition but local (session) state.

> Anyway, if people will try to find this feature on Google, then
> probably use keywords "session variables", so maybe my preference of
> more technical terminology is obscure and not practical, and the name
> "session variables" can be more practical for other people.
Hmmm, maybe.

> If I use the system used for GTT - then the exact name can be "Global
> Session Variable". Can we use this name? Or shortly just Session
> Variables because we don't support local session variables now.

So a "local variable" would be defined just for a given session, just
like a temporary table? Wouldn't that have the same issues with catalog
bloat as temporary tables?

I'd probably vote for "session variables". We can call it local/global
session variables in the future, if we end up implementing that.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/6/21 04:45, Pavel Stehule wrote:
> Hi
>
> st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
> <tomas(dot)vondra(at)enterprisedb(dot)com <mailto:tomas(dot)vondra(at)enterprisedb(dot)com>>
> napsal:
>
> Hi,
>
> I took a quick look at the latest patch version. In general the patch
> looks pretty complete and clean, and for now I have only some basic
> comments. The attached patch tweaks some of this, along with a couple
> additional minor changes that I'll not discuss here.
>
>
> 1) Not sure why we need to call this "schema variables". Most objects
> are placed in a schema, and we don't say "schema tables" for example.
> And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
> inconsistent.
>
>
> Yes, there is inconsistency, but I think it is necessary. The name
> "variable" is too generic. Theoretically we can use other adjectives
> like session variables or global variables and the name will be valid.
> But it doesn't describe the fundamentals of design. This is similar to
> the package's variables from PL/SQL. These variables are global,
> session's variables too. But the usual name is "package variables". So
> schema variables are assigned to schemes, and I think a good name can be
> "schema variables". But it is not necessary to repeat keyword schema in
> the CREATE COMMAND.
>
> My opinion is not too strong in this case, and I can accept just
> "variables" or "session's variables" or "global variables", but I am not
> sure if these names describe this feature well, because still they are
> too generic. There are too many different implementations of session
> global variables (see PL/SQL or T-SQL or DB2).
>

OK. "Session variable" seems better to me, but I'm not sure how well
that matches other databases. I'm not sure how much should we feel
constrained by naming in other databases, though.

>
> The docs actually use "Global variables" in one place for some reason.
>
>
> 2) I find this a bit confusing:
>
> SELECT non_existent_variable;
> test=# select s;
> ERROR:  column "non_existent_variable" does not exist
> LINE 1: select non_existent_variable;
>
> I wonder if this means using SELECT to read variables is a bad idea, and
> we should have a separate command, just like we have LET (instead of
> just using UPDATE in some way).
>
>
> I am sure so I want to use variables in SELECTs. One interesting case is
> using variables in RLS.
>

How much more complicated would it be without the SELECT?

> I prefer to fix this error message to "column or variable ... does not
> exist"
>

Not sure it's a good idea to make the error message more ambiguous. Most
people won't use variables at all, and the message will be less clear
for them.

regards

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

ne 7. 11. 2021 v 22:36 odesílatel Tomas Vondra <
tomas(dot)vondra(at)enterprisedb(dot)com> napsal:

> On 11/6/21 04:45, Pavel Stehule wrote:
> > Hi
> >
> > st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
> > <tomas(dot)vondra(at)enterprisedb(dot)com <mailto:tomas(dot)vondra(at)enterprisedb(dot)com>>
> > napsal:
> >
> > Hi,
> >
> > I took a quick look at the latest patch version. In general the patch
> > looks pretty complete and clean, and for now I have only some basic
> > comments. The attached patch tweaks some of this, along with a couple
> > additional minor changes that I'll not discuss here.
> >
> >
> > 1) Not sure why we need to call this "schema variables". Most objects
> > are placed in a schema, and we don't say "schema tables" for example.
> > And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a
> bit
> > inconsistent.
> >
> >
> > Yes, there is inconsistency, but I think it is necessary. The name
> > "variable" is too generic. Theoretically we can use other adjectives
> > like session variables or global variables and the name will be valid.
> > But it doesn't describe the fundamentals of design. This is similar to
> > the package's variables from PL/SQL. These variables are global,
> > session's variables too. But the usual name is "package variables". So
> > schema variables are assigned to schemes, and I think a good name can be
> > "schema variables". But it is not necessary to repeat keyword schema in
> > the CREATE COMMAND.
> >
> > My opinion is not too strong in this case, and I can accept just
> > "variables" or "session's variables" or "global variables", but I am not
> > sure if these names describe this feature well, because still they are
> > too generic. There are too many different implementations of session
> > global variables (see PL/SQL or T-SQL or DB2).
> >
>
> OK. "Session variable" seems better to me, but I'm not sure how well
> that matches other databases. I'm not sure how much should we feel
> constrained by naming in other databases, though.
>

session variables is generic term - there are big differences already -
T-SQL versus PL/SQL or SQL+ or DB2

> >
> > The docs actually use "Global variables" in one place for some
> reason.
> >
> >
> > 2) I find this a bit confusing:
> >
> > SELECT non_existent_variable;
> > test=# select s;
> > ERROR: column "non_existent_variable" does not exist
> > LINE 1: select non_existent_variable;
> >
> > I wonder if this means using SELECT to read variables is a bad idea,
> and
> > we should have a separate command, just like we have LET (instead of
> > just using UPDATE in some way).
> >
> >
> > I am sure so I want to use variables in SELECTs. One interesting case is
> > using variables in RLS.
> >
>
> How much more complicated would it be without the SELECT?
>

It is not too complicated, just you want to introduce SELECT2. The sense of
session variables is to be used. Has no sense to hold a value on a server
without the possibility to use it.

Session variables can be used as global variables in PL/pgSQL. If you
cannot use it in SQL expressions, then you need to copy it to a local
variable, and then you can use it. That cannot work. This design is a
replacement of a untyped not nullable slow workaround based on GUC, there
is a necessity to use it in SQL.

> > I prefer to fix this error message to "column or variable ... does not
> > exist"
> >
>
> Not sure it's a good idea to make the error message more ambiguous. Most
> people won't use variables at all, and the message will be less clear
> for them.
>

Yes, there is new complexity. But it is an analogy with variables in
PL/pgSQL with all benefits and negatives. You don't want to use dynamic SQL
everywhere you use PL/pgSQL variables.

There are more cases than RLS in SQL

1. hold value in session (for interactive work or for non interactive
scripts). Sometimes you want to reuse value - we can now use CTE or
temporary tables. But in this case you have to store relation, you cannot
store value, that can be used as a query parameter.

2. allow safe and effective parametrization of SQL scripts, and copy value
from client side to server side (there is not risk of SQL injection).

run script with parameter -v xx=10

```
create temp variable xx as int;
set xx = :`xx`;
do $$
.. -- I can work with variable xx on server side

...

$$

This is complement to client side variables - the advantage is possibility
to use outside psql, the are type, and the metadata can be permanent.

3. you can share value by PL environments (and by possible clients). But
this sharing is secure - the rules are the same like holding value in an
table.

Session variables increase complexity a little bit, but increases
possibilities and comfort for developers that use databases directly. The
analogy with PL/pgSQL variables is well, jut you are not limited to
PL/pgSQL scope.

Regards

Pavel

>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

On Sun, Nov 07, 2021 at 10:14:00PM +0100, Tomas Vondra wrote:
> I'd probably vote for "session variables". We can call it local/global
> session variables in the future, if we end up implementing that.

By chance, I ran into this pre-existing use of the phrase "session variable".
introduced since 8fbef1090:

doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command> does not process session variables as specified by

That's the *only* use of that phrase, but you'd have to change it to something
like ".. does not process role-specific variables a specified by ..".

--
Justin

Hi Justin

so 6. 11. 2021 v 2:39 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Wed, Nov 03, 2021 at 02:05:02PM +0100, Tomas Vondra wrote:
> > 3) I've reworded / tweaked a couple places in the docs, but this really
> > needs a native speaker - I don't have a very good "feeling" for this
> > technical language so it's probably still quite cumbersome.
>
> On Daniel's suggestion, I have reviewed the docs, and then proofread the
> rest
> of the patch. My amendments are in 0003.
>

Thank you for review and fixes, I try to complete some version for next
work, and looks so your patch 0001 is broken

gedit reports to me broken unicode \A0\A0\A0\A0\A0

my last patch has 276KB and your patch has 293KB?

Thank you

Pavel

> --
> Justin
>

>
> my last patch has 276KB and your patch has 293KB?
>

Please, can you resend your version of patch 0001?

Thank you

Pavel

On Mon, Nov 15, 2021 at 09:00:13PM +0100, Pavel Stehule wrote:
> Thank you for review and fixes, I try to complete some version for next
> work, and looks so your patch 0001 is broken
>
> gedit reports to me broken unicode \A0\A0\A0\A0\A0
>
> my last patch has 276KB and your patch has 293KB?

On Mon, Nov 15, 2021 at 09:06:08PM +0100, Pavel Stehule wrote:
> >
> > my last patch has 276KB and your patch has 293KB?
>
> Please, can you resend your version of patch 0001?

https://www.postgresql.org/message-id/[email protected]

0001 is exactly your patch applied to HEAD, and 0002 are Tomas' changes
relative to your patch.

0003 is my contribution on top. My intent is that you wouldn't apply 0001, but
rather apply my 0003 on top of your existing branch, and then review 0002/0003,
and then squish the changes into your patch.

I see the 0xa0 stuff in your original patch before my changes, but I'm not sure
what went wrong.

Let me know if you have any issue applying my changes on top of your existing,
local branch ?

--
Justin

po 15. 11. 2021 v 21:23 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Mon, Nov 15, 2021 at 09:00:13PM +0100, Pavel Stehule wrote:
> > Thank you for review and fixes, I try to complete some version for next
> > work, and looks so your patch 0001 is broken
> >
> > gedit reports to me broken unicode \A0\A0\A0\A0\A0
> >
> > my last patch has 276KB and your patch has 293KB?
>
> On Mon, Nov 15, 2021 at 09:06:08PM +0100, Pavel Stehule wrote:
> > >
> > > my last patch has 276KB and your patch has 293KB?
> >
> > Please, can you resend your version of patch 0001?
>
> https://www.postgresql.org/message-id/[email protected]
>
> 0001 is exactly your patch applied to HEAD, and 0002 are Tomas' changes
> relative to your patch.
>
> 0003 is my contribution on top. My intent is that you wouldn't apply
> 0001, but
> rather apply my 0003 on top of your existing branch, and then review
> 0002/0003,
> and then squish the changes into your patch.
>
> I see the 0xa0 stuff in your original patch before my changes, but I'm not
> sure
> what went wrong.
>
> Let me know if you have any issue applying my changes on top of your
> existing,
> local branch ?
>

It is ok, I was able to apply all your patches to my local branch

Regards

Pavel

>
> --
> Justin
>

Hi

8) immutable variables without a default value
>
> IMO this case should not be allowed. On 2021/08/29 you wrote:
>
> I thought about this case, and I have one scenario, where this
> behaviour can be useful. When the variable is declared as IMMUTABLE
> NOT NULL without not null default, then any access to the content of
> the variable has to fail. I think it can be used for detection,
> where and when the variable is first used. So this behavior is
> allowed just because I think, so this feature can be interesting for
> debugging. If this idea is too strange, I have no problem to disable
> this case.
>

I checked code, and this case is disallowed already

postgres=# CREATE IMMUTABLE VARIABLE xx AS int NOT NULL;
ERROR: IMMUTABLE NOT NULL variable requires default expression

Regards

Pavel

Hi

po 15. 11. 2021 v 21:23 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Mon, Nov 15, 2021 at 09:00:13PM +0100, Pavel Stehule wrote:
> > Thank you for review and fixes, I try to complete some version for next
> > work, and looks so your patch 0001 is broken
> >
> > gedit reports to me broken unicode \A0\A0\A0\A0\A0
> >
> > my last patch has 276KB and your patch has 293KB?
>
> On Mon, Nov 15, 2021 at 09:06:08PM +0100, Pavel Stehule wrote:
> > >
> > > my last patch has 276KB and your patch has 293KB?
> >
> > Please, can you resend your version of patch 0001?
>
> https://www.postgresql.org/message-id/[email protected]
>
> 0001 is exactly your patch applied to HEAD, and 0002 are Tomas' changes
> relative to your patch.
>
> 0003 is my contribution on top. My intent is that you wouldn't apply
> 0001, but
> rather apply my 0003 on top of your existing branch, and then review
> 0002/0003,
> and then squish the changes into your patch.
>
> I see the 0xa0 stuff in your original patch before my changes, but I'm not
> sure
> what went wrong.
>
> Let me know if you have any issue applying my changes on top of your
> existing,
> local branch ?
>

I am sending new versions of patches.

I hope I solved all Tomas's objections.

1. The schema variables were renamed to session variables
2. I fixed issues related to creating, dropping variables under
subtransactions + regress tests
3. I fixed issues in pg_dump + regress tests

Regards

Pavel

> --
> Justin
>

Op 19-12-2021 om 07:23 schreef Pavel Stehule:

>
> I am sending new versions of patches.
>
> I hope I solved all Tomas's objections.
>
> 1. The schema variables were renamed to session variables
> 2. I fixed issues related to creating, dropping variables under
> subtransactions + regress tests
> 3. I fixed issues in pg_dump + regress tests
>

> [0001-schema-variables-20211219.patch]
> [0002-schema-variables-20211219.patch]

Hi Pavel,

I get an error during test 'session_variables'.

(on the upside, my own little testsuite runs without error)

thanks,

Erik Rijkers

ne 19. 12. 2021 v 8:09 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> Op 19-12-2021 om 07:23 schreef Pavel Stehule:
>
> >
> > I am sending new versions of patches.
> >
> > I hope I solved all Tomas's objections.
> >
> > 1. The schema variables were renamed to session variables
> > 2. I fixed issues related to creating, dropping variables under
> > subtransactions + regress tests
> > 3. I fixed issues in pg_dump + regress tests
> >
>
> > [0001-schema-variables-20211219.patch]
> > [0002-schema-variables-20211219.patch]
>
> Hi Pavel,
>
> I get an error during test 'session_variables'.
>
> (on the upside, my own little testsuite runs without error)
>
> thanks,
>

please, can you send me regress diff?

Regards

Pavel

> Erik Rijkers
>
>
>
>
>
>
>
>
>
>

ne 19. 12. 2021 v 8:13 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> ne 19. 12. 2021 v 8:09 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:
>
>> Op 19-12-2021 om 07:23 schreef Pavel Stehule:
>>
>> >
>> > I am sending new versions of patches.
>> >
>> > I hope I solved all Tomas's objections.
>> >
>> > 1. The schema variables were renamed to session variables
>> > 2. I fixed issues related to creating, dropping variables under
>> > subtransactions + regress tests
>> > 3. I fixed issues in pg_dump + regress tests
>> >
>>
>> > [0001-schema-variables-20211219.patch]
>> > [0002-schema-variables-20211219.patch]
>>
>> Hi Pavel,
>>
>> I get an error during test 'session_variables'.
>>
>> (on the upside, my own little testsuite runs without error)
>>
>> thanks,
>>
>
> please, can you send me regress diff?
>

I see the problem now, the test contains username, and that is wrong.

Schema | Name | Type | Is nullable | Is mutable | Default | Owner |
Transactional end action | Access privileges | Description
-----------+------+---------+-------------+------------+---------+-------+--------------------------+------------------------+-------------
- svartest | var1 | numeric | t | t | | pavel | | pavel=SW/pavel +|
- | | | | | | | | var_test_role=SW/pavel |
+----------+------+---------+-------------+------------+---------+----------+--------------------------+---------------------------+-------------
+ svartest | var1 | numeric | t | t | | appveyor | | appveyor=SW/appveyor
+|
+ | | | | | | | | var_test_role=SW/appveyor |
(1 row)
REVOKE ALL ON VARIABLE var1 FROM var_test_role;

I have to remove this test

Pavel

Regards
>
> Pavel
>
>
>
>> Erik Rijkers
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>

Op 19-12-2021 om 08:13 schreef Pavel Stehule:
>
>
> ne 19. 12. 2021 v 8:09 odesílatel Erik Rijkers <er(at)xs4all(dot)nl
> >
>
>  > [0001-schema-variables-20211219.patch]
>  > [0002-schema-variables-20211219.patch]
>
> Hi Pavel,
>
> I get an error during test 'session_variables'.
>
> (on the upside, my own little testsuite runs without error)
>
> thanks,
>
>
> please, can you send me regress diff?
>

I did attach it but if you did not receive it, see also cfbot, especially

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.156992

Erik

ne 19. 12. 2021 v 8:23 odesílatel Erikjan Rijkers <er(at)xs4all(dot)nl> napsal:

> Op 19-12-2021 om 08:13 schreef Pavel Stehule:
> >
> >
> > ne 19. 12. 2021 v 8:09 odesílatel Erik Rijkers <er(at)xs4all(dot)nl
> > >
> >
> > > [0001-schema-variables-20211219.patch]
> > > [0002-schema-variables-20211219.patch]
> >
> > Hi Pavel,
> >
> > I get an error during test 'session_variables'.
> >
> > (on the upside, my own little testsuite runs without error)
> >
> > thanks,
> >
> >
> > please, can you send me regress diff?
> >
>
> I did attach it but if you did not receive it, see also cfbot, especially
>
>
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.156992

second try

I removed badly written tests

Pavel

>
>
> Erik
>
>

> [0001-schema-variables-20211219-2.patch]
> [0002-schema-variables-20211219-2.patch]

Hi Pavel,

You said earlier
> 1. The schema variables were renamed to session variable

But I still see:
$ grep -Eic 'schema variable' postgres.html
15

(postgres.html from 'make postgres.html')

So that rename doesn't seem finished.

Erik

Hi

ne 19. 12. 2021 v 11:10 odesílatel Erik Rijkers <er(at)xs4all(dot)nl> napsal:

> > [0001-schema-variables-20211219-2.patch]
> > [0002-schema-variables-20211219-2.patch]
>
> Hi Pavel,
>
> You said earlier
> > 1. The schema variables were renamed to session variable
>
> But I still see:
> $ grep -Eic 'schema variable' postgres.html
> 15
>
> (postgres.html from 'make postgres.html')
>
> So that rename doesn't seem finished.
>

Yes, I forgot some changes, and more, there was a bogus regress result
file. Thank you for rechecking.

I am sending cleaned patches

Regards

Pavel

>
> Erik
>
>
>
>

I don't understand what 0002 patch does relative to the 0001 patch.
Is 0002 to change the error messages from "schema variables" to "session
variables" , in a separate commit to show that the main patch doesn't change
regression results ? Could you add commit messages ?

I mentioned before that there's a pre-existing use of the phrase "session
variable", which you should change to something else:

origin:doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command> does not process session variables as specified by
origin:doc/src/sgml/ref/set_role.sgml- the role's <link linkend="sql-alterrole"><command>ALTER ROLE</command></link> settings; this only happens during
origin:doc/src/sgml/ref/set_role.sgml- login.

Maybe "session variable" should be added to the glossary.

The new tests crash if debug_discard_caches=on.

2021-12-20 16:15:44.476 CST postmaster[7478] LOG: server process (PID 7657) was terminated by signal 6: Aborted
2021-12-20 16:15:44.476 CST postmaster[7478] DETAIL: Failed process was running: DISCARD VARIABLES;

TRAP: FailedAssertion("sessionvars", File: "sessionvariable.c", Line: 270, PID: 7657)

#2 0x0000564858a4f1a8 in ExceptionalCondition (conditionName=conditionName(at)entry=0x564858b8626d "sessionvars", errorType=errorType(at)entry=0x564858aa700b "FailedAssertion",
fileName=fileName(at)entry=0x564858b86234 "sessionvariable.c", lineNumber=lineNumber(at)entry=270) at assert.c:69
#3 0x000056485874fec6 in sync_sessionvars_xact_callback (event=<optimized out>, arg=<optimized out>) at sessionvariable.c:270
#4 sync_sessionvars_xact_callback (event=<optimized out>, arg=<optimized out>) at sessionvariable.c:253
#5 0x000056485868030a in CallXactCallbacks (event=XACT_EVENT_PRE_COMMIT) at xact.c:3644
#6 CommitTransaction () at xact.c:2178
#7 0x0000564858681975 in CommitTransactionCommand () at xact.c:3043
#8 0x000056485892b7a9 in finish_xact_command () at postgres.c:2722
#9 0x000056485892dc5b in finish_xact_command () at postgres.c:2720
#10 exec_simple_query () at postgres.c:1240
#11 0x000056485892f70a in PostgresMain () at postgres.c:4498
#12 0x000056485889a479 in BackendRun (port=<optimized out>, port=<optimized out>) at postmaster.c:4594
#13 BackendStartup (port=<optimized out>) at postmaster.c:4322
#14 ServerLoop () at postmaster.c:1802
#15 0x000056485889b47c in PostmasterMain () at postmaster.c:1474
#16 0x00005648585c60c0 in main (argc=5, argv=0x564858e553f0) at main.c:198

Hi

út 21. 12. 2021 v 0:09 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> I don't understand what 0002 patch does relative to the 0001 patch.
> Is 0002 to change the error messages from "schema variables" to "session
> variables" , in a separate commit to show that the main patch doesn't
> change
> regression results ? Could you add commit messages ?
>
> I mentioned before that there's a pre-existing use of the phrase "session
> variable", which you should change to something else:
>
> origin:doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command> does
> not process session variables as specified by
> origin:doc/src/sgml/ref/set_role.sgml- the role's <link
> linkend="sql-alterrole"><command>ALTER ROLE</command></link> settings;
> this only happens during
> origin:doc/src/sgml/ref/set_role.sgml- login.
>
> Maybe "session variable" should be added to the glossary.
>
> The new tests crash if debug_discard_caches=on.
>
> 2021-12-20 16:15:44.476 CST postmaster[7478] LOG: server process (PID
> 7657) was terminated by signal 6: Aborted
> 2021-12-20 16:15:44.476 CST postmaster[7478] DETAIL: Failed process was
> running: DISCARD VARIABLES;
>

How do you inject this parameter to regress tests?

Regards

Pavel

> TRAP: FailedAssertion("sessionvars", File: "sessionvariable.c", Line: 270,
> PID: 7657)
>
> #2 0x0000564858a4f1a8 in ExceptionalCondition
> (conditionName=conditionName(at)entry=0x564858b8626d "sessionvars",
> errorType=errorType(at)entry=0x564858aa700b "FailedAssertion",
> fileName=fileName(at)entry=0x564858b86234 "sessionvariable.c",
> lineNumber=lineNumber(at)entry=270) at assert.c:69
> #3 0x000056485874fec6 in sync_sessionvars_xact_callback (event=<optimized
> out>, arg=<optimized out>) at sessionvariable.c:270
> #4 sync_sessionvars_xact_callback (event=<optimized out>, arg=<optimized
> out>) at sessionvariable.c:253
> #5 0x000056485868030a in CallXactCallbacks (event=XACT_EVENT_PRE_COMMIT)
> at xact.c:3644
> #6 CommitTransaction () at xact.c:2178
> #7 0x0000564858681975 in CommitTransactionCommand () at xact.c:3043
> #8 0x000056485892b7a9 in finish_xact_command () at postgres.c:2722
> #9 0x000056485892dc5b in finish_xact_command () at postgres.c:2720
> #10 exec_simple_query () at postgres.c:1240
> #11 0x000056485892f70a in PostgresMain () at postgres.c:4498
> #12 0x000056485889a479 in BackendRun (port=<optimized out>,
> port=<optimized out>) at postmaster.c:4594
> #13 BackendStartup (port=<optimized out>) at postmaster.c:4322
> #14 ServerLoop () at postmaster.c:1802
> #15 0x000056485889b47c in PostmasterMain () at postmaster.c:1474
> #16 0x00005648585c60c0 in main (argc=5, argv=0x564858e553f0) at main.c:198
>

On Tue, Dec 21, 2021 at 01:29:00PM +0100, Pavel Stehule wrote:
> Hi
>
> út 21. 12. 2021 v 0:09 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
> napsal:
>
> > I don't understand what 0002 patch does relative to the 0001 patch.
> > Is 0002 to change the error messages from "schema variables" to "session
> > variables" , in a separate commit to show that the main patch doesn't
> > change
> > regression results ? Could you add commit messages ?
> >
> > I mentioned before that there's a pre-existing use of the phrase "session
> > variable", which you should change to something else:
> >
> > origin:doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command> does
> > not process session variables as specified by
> > origin:doc/src/sgml/ref/set_role.sgml- the role's <link
> > linkend="sql-alterrole"><command>ALTER ROLE</command></link> settings;
> > this only happens during
> > origin:doc/src/sgml/ref/set_role.sgml- login.
> >
> > Maybe "session variable" should be added to the glossary.
> >
> > The new tests crash if debug_discard_caches=on.
> >
> > 2021-12-20 16:15:44.476 CST postmaster[7478] LOG: server process (PID
> > 7657) was terminated by signal 6: Aborted
> > 2021-12-20 16:15:44.476 CST postmaster[7478] DETAIL: Failed process was
> > running: DISCARD VARIABLES;
>
> How do you inject this parameter to regress tests?

You can run PGOPTIONS='-c debug_invalidate_caches=1' make check

I used make installcheck against a running instance where I'd used
ALTER SYSTEM SET debug_discard_caches=on.

You can also manually run psql against the .sql file itself.
...which is a good idea since this causes the regression tests take hours.

Or just add SET debug_discard_caches=on to your .sql file.

--
Justin

út 21. 12. 2021 v 13:36 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Tue, Dec 21, 2021 at 01:29:00PM +0100, Pavel Stehule wrote:
> > Hi
> >
> > út 21. 12. 2021 v 0:09 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
> > napsal:
> >
> > > I don't understand what 0002 patch does relative to the 0001 patch.
> > > Is 0002 to change the error messages from "schema variables" to
> "session
> > > variables" , in a separate commit to show that the main patch doesn't
> > > change
> > > regression results ? Could you add commit messages ?
> > >
> > > I mentioned before that there's a pre-existing use of the phrase
> "session
> > > variable", which you should change to something else:
> > >
> > > origin:doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command>
> does
> > > not process session variables as specified by
> > > origin:doc/src/sgml/ref/set_role.sgml- the role's <link
> > > linkend="sql-alterrole"><command>ALTER ROLE</command></link> settings;
> > > this only happens during
> > > origin:doc/src/sgml/ref/set_role.sgml- login.
> > >
> > > Maybe "session variable" should be added to the glossary.
> > >
> > > The new tests crash if debug_discard_caches=on.
> > >
> > > 2021-12-20 16:15:44.476 CST postmaster[7478] LOG: server process (PID
> > > 7657) was terminated by signal 6: Aborted
> > > 2021-12-20 16:15:44.476 CST postmaster[7478] DETAIL: Failed process
> was
> > > running: DISCARD VARIABLES;
> >
> > How do you inject this parameter to regress tests?
>
> You can run PGOPTIONS='-c debug_invalidate_caches=1' make check
>
> I used make installcheck against a running instance where I'd used
> ALTER SYSTEM SET debug_discard_caches=on.
>
> You can also manually run psql against the .sql file itself.
> ...which is a good idea since this causes the regression tests take hours.
>
> Or just add SET debug_discard_caches=on to your .sql file.
>

ok thank you

I'll try it.

> --
> Justin
>

út 21. 12. 2021 v 0:09 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> I don't understand what 0002 patch does relative to the 0001 patch.
> Is 0002 to change the error messages from "schema variables" to "session
> variables" , in a separate commit to show that the main patch doesn't
> change
> regression results ? Could you add commit messages ?
>
>
done

> I mentioned before that there's a pre-existing use of the phrase "session
> variable", which you should change to something else:
>
> origin:doc/src/sgml/ref/set_role.sgml: <command>SET ROLE</command> does
> not process session variables as specified by
> origin:doc/src/sgml/ref/set_role.sgml- the role's <link
> linkend="sql-alterrole"><command>ALTER ROLE</command></link> settings;
> this only happens during
> origin:doc/src/sgml/ref/set_role.sgml- login.
>

changed

> Maybe "session variable" should be added to the glossary.
>

done

> The new tests crash if debug_discard_caches=on.
>
> 2021-12-20 16:15:44.476 CST postmaster[7478] LOG: server process (PID
> 7657) was terminated by signal 6: Aborted
> 2021-12-20 16:15:44.476 CST postmaster[7478] DETAIL: Failed process was
> running: DISCARD VARIABLES;
>
> TRAP: FailedAssertion("sessionvars", File: "sessionvariable.c", Line: 270,
> PID: 7657)
>
> #2 0x0000564858a4f1a8 in ExceptionalCondition
> (conditionName=conditionName(at)entry=0x564858b8626d "sessionvars",
> errorType=errorType(at)entry=0x564858aa700b "FailedAssertion",
> fileName=fileName(at)entry=0x564858b86234 "sessionvariable.c",
> lineNumber=lineNumber(at)entry=270) at assert.c:69
> #3 0x000056485874fec6 in sync_sessionvars_xact_callback (event=<optimized
> out>, arg=<optimized out>) at sessionvariable.c:270
> #4 sync_sessionvars_xact_callback (event=<optimized out>, arg=<optimized
> out>) at sessionvariable.c:253
> #5 0x000056485868030a in CallXactCallbacks (event=XACT_EVENT_PRE_COMMIT)
> at xact.c:3644
> #6 CommitTransaction () at xact.c:2178
> #7 0x0000564858681975 in CommitTransactionCommand () at xact.c:3043
> #8 0x000056485892b7a9 in finish_xact_command () at postgres.c:2722
> #9 0x000056485892dc5b in finish_xact_command () at postgres.c:2720
> #10 exec_simple_query () at postgres.c:1240
> #11 0x000056485892f70a in PostgresMain () at postgres.c:4498
> #12 0x000056485889a479 in BackendRun (port=<optimized out>,
> port=<optimized out>) at postmaster.c:4594
> #13 BackendStartup (port=<optimized out>) at postmaster.c:4322
> #14 ServerLoop () at postmaster.c:1802
> #15 0x000056485889b47c in PostmasterMain () at postmaster.c:1474
> #16 0x00005648585c60c0 in main (argc=5, argv=0x564858e553f0) at main.c:198
>

attached version was ok with this setting

Regards

Pavel

Hi

new update - I found an error in checking ambiguous columns - the tupdesc
was badly released by FreeTupleDesc. I fixed this issue and did a new
related regress test to cover this path.

Regards

Nice holidays

Pavel

---------- Forwarded message ---------
Od: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Date: po 27. 12. 2021 v 5:30
Subject: Re: Schema variables - new implementation for Postgres 15
To: Justin Pryzby <pryzby(at)telsasoft(dot)com>

Hi

ne 26. 12. 2021 v 15:43 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> > > Maybe "session variable" should be added to the glossary.
> >
> > done
>
> + A persistent database object that holds an value in session memory.
> + This memory is not shared across sessions, and after session end,
> this
> + memory (the value) is released. The access (read or write) to
> session variables
> + is controlled by access rigths similary to other database object
> access rigts.
>
> an value => a value
> rigths => rights
> rigts => rights
>

fixed

Regards

Pavel

Hi

rebase

Regards

Pavel

On Wed, 3 Nov 2021 at 13:05, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> wrote:
>
> 2) I find this a bit confusing:
>
> SELECT non_existent_variable;
> test=# select s;
> ERROR: column "non_existent_variable" does not exist
> LINE 1: select non_existent_variable;
>
> I wonder if this means using SELECT to read variables is a bad idea, and
> we should have a separate command, just like we have LET (instead of
> just using UPDATE in some way).
>

Hmm. This way of reading variables worries me for a different reason
-- I think it makes it all too easy to break existing applications by
inadvertently (or deliberately) defining variables that conflict with
column names referred to in existing queries.

For example, if I define a variable called "relkind", then psql's \sv
meta-command is broken because the query it performs can't distinguish
between the column and the variable.

Similarly, there's ambiguity between alias.colname and
schema.variablename. So, for example, if I do the following:

CREATE SCHEMA n;
CREATE VARIABLE n.nspname AS int;

then lots of things are broken, including pg_dump and a number of psql
meta-commands. I don't think it's acceptable to make it so easy for a
user to break the system in this way.

Those are examples that a malicious user might use, but even without
such examples, I think it would be far too easy to inadvertently break
a large application by defining a variable that conflicted with a
column name you didn't know about.

Regards,
Dean

čt 13. 1. 2022 v 13:54 odesílatel Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
napsal:

> On Wed, 3 Nov 2021 at 13:05, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
> wrote:
> >
> > 2) I find this a bit confusing:
> >
> > SELECT non_existent_variable;
> > test=# select s;
> > ERROR: column "non_existent_variable" does not exist
> > LINE 1: select non_existent_variable;
> >
> > I wonder if this means using SELECT to read variables is a bad idea, and
> > we should have a separate command, just like we have LET (instead of
> > just using UPDATE in some way).
> >
>
> Hmm. This way of reading variables worries me for a different reason
> -- I think it makes it all too easy to break existing applications by
> inadvertently (or deliberately) defining variables that conflict with
> column names referred to in existing queries.
>
> For example, if I define a variable called "relkind", then psql's \sv
> meta-command is broken because the query it performs can't distinguish
> between the column and the variable.
>
> Similarly, there's ambiguity between alias.colname and
> schema.variablename. So, for example, if I do the following:
>
> CREATE SCHEMA n;
> CREATE VARIABLE n.nspname AS int;
>
> then lots of things are broken, including pg_dump and a number of psql
> meta-commands. I don't think it's acceptable to make it so easy for a
> user to break the system in this way.
>
> Those are examples that a malicious user might use, but even without
> such examples, I think it would be far too easy to inadvertently break
> a large application by defining a variable that conflicted with a
> column name you didn't know about.
>

This is a valid issue, and it should be solved, or reduce a risk

I see two possibilities

a) easy solution can be implementation of other conflict strategy -
variables have lower priority than tables with possibility to raise
warnings if some identifiers are ambiguous. This is easy to implement, and
with warning I think there should not be some unwanted surprises for
developers. This is safe in meaning - no variable can break any query.

b) harder implementation (but I long think about it) can be implementation
of schema scope access. It can be used for implementation of schema private
objects. It doesn't solve the described issue, but it can reduce the risk
of collision just for one schema.

Both possibilities can be implemented together - but the @b solution should
be implemented from zero - and it is more generic concept, and then I
prefer @a

Dean, can @a work for you?

Regards

Pavel

> Regards,
> Dean
>

On Thu, Jan 13, 2022, at 18:24, Dean Rasheed wrote:
> Those are examples that a malicious user might use, but even without
> such examples, I think it would be far too easy to inadvertently break
> a large application by defining a variable that conflicted with a
> column name you didn't know about.

I think there is also a readability problem with the non-locality of this feature.

I think it would be better to have an explicit namespace for these global variables, so that when reading code, they would stand-out.
As a bonus, that would also solve the risk of breaking code, as you pointed out.

Most code should never need any global variables at all, so in the rare occasions when they are needed, I think it's perfectly fine if some more verbose fully-qualified syntax was needed to use them, rather than to pollute the namespace and risk breaking code.

I want to bring up an idea presented earlier in a different thread:

How about exploiting reserved SQL keywords followed by a dot, as special labels?

This could solve the problem with this patch, as well as the other root label patch to access function parameters.

It's an unorthodox idea, but due to legacy, I think we need to be creative, if we want a safe solution with no risk of breaking any code, which I think should be a requirement.

Taking inspiration from Javascript, how about using the SQL reserved keyword "window"?
In Javascript, "window.variableName" means that the variable variableName declared at the global scope.

Furthermore:

"from" could be used to access function/procedure IN parameters.
"to" could be used to access function OUT parameters.
"from" or "to" could be used to access function INOUT parameters.

Examples:

SELECT u.user_id
INTO to.user_id
FROM users u
WHERE u.username = from.username;

-- After authentication, the authenticated user_id could be stored as a global variable:
window.user_id := to.user_id;

-- The authenticated user_id could then be used in queries that should filter on user_id:
SELECT o.order_id
FROM orders o
WHERE o.user_id = window.user_id;

This would require endorsement from the SQL committee of course, otherwise we would face problems if they suddenly would introduce syntax where a reserved keyword could be followed by a dot.

I think from a readability perspective, it works, since the different meanings can be distinguished by writing one in UPPERCASE and the other in lowercase.

/Joel

čt 13. 1. 2022 v 15:29 odesílatel Joel Jacobson <joel(at)compiler(dot)org> napsal:

> On Thu, Jan 13, 2022, at 18:24, Dean Rasheed wrote:
> > Those are examples that a malicious user might use, but even without
> > such examples, I think it would be far too easy to inadvertently break
> > a large application by defining a variable that conflicted with a
> > column name you didn't know about.
>
> I think there is also a readability problem with the non-locality of this
> feature.
>
> I think it would be better to have an explicit namespace for these global
> variables, so that when reading code, they would stand-out.
> As a bonus, that would also solve the risk of breaking code, as you
> pointed out.
>
> Most code should never need any global variables at all, so in the rare
> occasions when they are needed, I think it's perfectly fine if some more
> verbose fully-qualified syntax was needed to use them, rather than to
> pollute the namespace and risk breaking code.
>

There are few absolutely valid use cases

1. scripting - currently used GUC instead session variables are slow, and
without types

2. RLS

3. Migration from Oracle - although I agree, so package variables are used
more times badly, it used there. And only in few times is possibility to
refactor code when you do migration from Oracle to Postgres, and there is
necessity to have session variables,

> I want to bring up an idea presented earlier in a different thread:
>
> How about exploiting reserved SQL keywords followed by a dot, as special
> labels?
>
> This could solve the problem with this patch, as well as the other root
> label patch to access function parameters.
>
> It's an unorthodox idea, but due to legacy, I think we need to be
> creative, if we want a safe solution with no risk of breaking any code,
> which I think should be a requirement.
>
> Taking inspiration from Javascript, how about using the SQL reserved
> keyword "window"?
> In Javascript, "window.variableName" means that the variable variableName
> declared at the global scope.
>

I cannot imagine how the "window" keyword can work in SQL context. In
Javascript "window" is an object - it is not a keyword, and it makes sense
in usual Javascript context inside HTML browsers.

Regards

Pavel

>
> Furthermore:
>
> "from" could be used to access function/procedure IN parameters.
> "to" could be used to access function OUT parameters.
> "from" or "to" could be used to access function INOUT parameters.
>
> Examples:
>
> SELECT u.user_id
> INTO to.user_id
> FROM users u
> WHERE u.username = from.username;
>
> -- After authentication, the authenticated user_id could be stored as a
> global variable:
> window.user_id := to.user_id;
>
> -- The authenticated user_id could then be used in queries that should
> filter on user_id:
> SELECT o.order_id
> FROM orders o
> WHERE o.user_id = window.user_id;
>
> This would require endorsement from the SQL committee of course, otherwise
> we would face problems if they suddenly would introduce syntax where a
> reserved keyword could be followed by a dot.
>
> I think from a readability perspective, it works, since the different
> meanings can be distinguished by writing one in UPPERCASE and the other in
> lowercase.
>
> /Joel
>

On Thu, Jan 13, 2022, at 20:12, Pavel Stehule wrote:
>I cannot imagine how the "window" keyword can work in SQL context. In Javascript "window" is an object - it is not a keyword, and it makes sense in usual Javascript context inside HTML browsers.

I was thinking since Javascript is by far the most known programming language, the "window" word would be familiar and easy to remember, but I agree, it's not perfect.

Hm, "values" would be nice, it's reserved in SQL:2016 [1] and in DB2/Mimer/MySQL/Oracle/SQL Server/Teradata [2], but unfortunately not in PostgreSQL [1], so perhaps not doable.

Syntax:

values.[schema name].[variable name]

[1] https://www.postgresql.org/docs/current/sql-keywords-appendix.html
[2] https://en.wikipedia.org/wiki/SQL_reserved_words

čt 13. 1. 2022 v 18:01 odesílatel Joel Jacobson <joel(at)compiler(dot)org> napsal:

> On Thu, Jan 13, 2022, at 20:12, Pavel Stehule wrote:
> >I cannot imagine how the "window" keyword can work in SQL context. In
> Javascript "window" is an object - it is not a keyword, and it makes sense
> in usual Javascript context inside HTML browsers.
>
> I was thinking since Javascript is by far the most known programming
> language, the "window" word would be familiar and easy to remember, but I
> agree, it's not perfect.
>

Mainly the "window" is just a global variable. It is not a special keyword.
So the syntax object.property is usual.

> Hm, "values" would be nice, it's reserved in SQL:2016 [1] and in
> DB2/Mimer/MySQL/Oracle/SQL Server/Teradata [2], but unfortunately not in
> PostgreSQL [1], so perhaps not doable.
>
> Syntax:
>
> values.[schema name].[variable name]
>

This doesn't help too much. This syntax is too long. It can solve the
described issue, but only when all three parts will be required, and
writing every time VALUES.schemaname.variablename is not too practical. And
if we require this three part identifier every time, then it can be used
with the already supported dbname.schemaname.varname. Almost all collisions
can be fixed by using a three part identifier. But it doesn't look too
handy.

I like the idea of prioritizing tables over variables with warnings when
collision is detected. It cannot break anything. And it allows to using
short identifiers when there is not collision. If somebody don't want to
any collision then can use schema "vars", "values", or what he/she likes.
It is near to your proposal - it is not too often so people use table alias
like "values" (although in EAV case it is possible).

> [1] https://www.postgresql.org/docs/current/sql-keywords-appendix.html
> [2] https://en.wikipedia.org/wiki/SQL_reserved_words
>
>

On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>
> I like the idea of prioritizing tables over variables with warnings when collision is detected. It cannot break anything. And it allows to using short identifiers when there is not collision.

Yeah, that seems OK, as long as it's clearly documented. I don't think
a warning is necessary.

(FWIW, testing with dbfiddle, that appears to match Db2's behaviour).

Regards,
Dean

čt 13. 1. 2022 v 19:23 odesílatel Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
napsal:

> On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >
> > I like the idea of prioritizing tables over variables with warnings when
> collision is detected. It cannot break anything. And it allows to using
> short identifiers when there is not collision.
>
> Yeah, that seems OK, as long as it's clearly documented. I don't think
> a warning is necessary.
>

The warning can be disabled by default, but I think it should be there.
This is a signal, so some in the database schema should be renamed. Maybe -
session_variables_ambiguity_warning.

> (FWIW, testing with dbfiddle, that appears to match Db2's behaviour).
>

Thank you for check

Regards

Pavel

> Regards,
> Dean
>

Hi,

On Thu, Jan 13, 2022 at 07:32:26PM +0100, Pavel Stehule wrote:
> čt 13. 1. 2022 v 19:23 odesílatel Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
> napsal:
>
> > On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > wrote:
> > >
> > > I like the idea of prioritizing tables over variables with warnings when
> > collision is detected. It cannot break anything. And it allows to using
> > short identifiers when there is not collision.
> >
> > Yeah, that seems OK, as long as it's clearly documented. I don't think
> > a warning is necessary.

What should be the behavior for a cached plan that uses a variable when a
conflicting relation is later created? I think that it should be the same as a
search_path change and the plan should be discarded.

> The warning can be disabled by default, but I think it should be there.
> This is a signal, so some in the database schema should be renamed. Maybe -
> session_variables_ambiguity_warning.

I agree that having a way to know that a variable has been bypassed can be
useful.

> > (FWIW, testing with dbfiddle, that appears to match Db2's behaviour).
> >
>
> Thank you for check

Do you know what's oracle's behavior on that?

I've been looking at the various dependency handling, and I noticed that
collation are ignored, while they're accepted syntax-wise:

=# create collation mycollation (locale = 'fr-FR', provider = 'icu');
CREATE COLLATION

=# create variable myvariable text collate mycollation;
CREATE VARIABLE

=# select classid::regclass, objid, objsubid, refclassid::regclass, refobjid, refobjsubid from pg_depend where classid::regclass::text = 'pg_variable' or refclassid::regclass::text = 'pg_variable';
classid | objid | objsubid | refclassid | refobjid | refobjsubid
-------------+-------+----------+--------------+----------+-------------
pg_variable | 16407 | 0 | pg_namespace | 2200 | 0
(1 row)

=# let myvariable = 'AA';
LET

=# select 'AA' collate "en-x-icu" < myvariable;
?column?
----------
f
(1 row)

=# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
ERROR: 42P21: collation mismatch between explicit collations "en-x-icu" and "mycollation"
LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...

So it's missing both dependency recording for variable's collation and also
teaching various code that variables can have a collation.

It's also missing some invalidation detection. For instance:

=# create variable myval text;
CREATE VARIABLE

=# let myval = 'pg_class';
LET

=# prepare s(text) as select relname from pg_class where relname = $1 or relname = myval;
PREPARE

=# set plan_cache_mode = force_generic_plan ;
SET

=# execute s ('');
relname
----------
pg_class
(1 row)

=# drop variable myval ;
DROP VARIABLE

=# create variable myval int;
CREATE VARIABLE

=# execute s ('');
ERROR: XX000: cache lookup failed for session variable 16408

The plan should have been discarded and the new plan should fail for type
problem.

Strangely, subsequent calls don't error out:

=# execute s('');
relname
---------
(0 rows)

But doing an explain shows that there's a problem:

=# explain execute s('');
ERROR: XX000: cache lookup failed for variable 16408

pá 14. 1. 2022 v 3:44 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Thu, Jan 13, 2022 at 07:32:26PM +0100, Pavel Stehule wrote:
> > čt 13. 1. 2022 v 19:23 odesílatel Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com
> >
> > napsal:
> >
> > > On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > > wrote:
> > > >
> > > > I like the idea of prioritizing tables over variables with warnings
> when
> > > collision is detected. It cannot break anything. And it allows to using
> > > short identifiers when there is not collision.
> > >
> > > Yeah, that seems OK, as long as it's clearly documented. I don't think
> > > a warning is necessary.
>
> What should be the behavior for a cached plan that uses a variable when a
> conflicting relation is later created? I think that it should be the same
> as a
> search_path change and the plan should be discarded.
>

This is a more generic problem - creating a new DDL object doesn't
invalidate plans.

https://www.postgresql.org/message-id/2589876.1641914327%40sss.pgh.pa.us

>
> > The warning can be disabled by default, but I think it should be there.
> > This is a signal, so some in the database schema should be renamed.
> Maybe -
> > session_variables_ambiguity_warning.
>
> I agree that having a way to know that a variable has been bypassed can be
> useful.
>
> > > (FWIW, testing with dbfiddle, that appears to match Db2's behaviour).
> > >
> >
> > Thank you for check
>
> Do you know what's oracle's behavior on that?
>
>
Oracle is very different, because package variables are not visible from
plain SQL. And change of interface invalidates dependent objects and
requires recompilation. So it is a little bit more sensitive. If I remember
well, the SQL identifiers have bigger priority than PL/SQL identifiers
(package variables), so proposed behavior is very similar to Oracle
behavior too. The risk of unwanted collision is reduced (on Oracle) by
local visibility of package variables, and availability of package
variables only in some environments.

>
> I've been looking at the various dependency handling, and I noticed that
> collation are ignored, while they're accepted syntax-wise:
>
> =# create collation mycollation (locale = 'fr-FR', provider = 'icu');
> CREATE COLLATION
>
> =# create variable myvariable text collate mycollation;
> CREATE VARIABLE
>
> =# select classid::regclass, objid, objsubid, refclassid::regclass,
> refobjid, refobjsubid from pg_depend where classid::regclass::text =
> 'pg_variable' or refclassid::regclass::text = 'pg_variable';
> classid | objid | objsubid | refclassid | refobjid | refobjsubid
> -------------+-------+----------+--------------+----------+-------------
> pg_variable | 16407 | 0 | pg_namespace | 2200 | 0
> (1 row)
>
> =# let myvariable = 'AA';
> LET
>
> =# select 'AA' collate "en-x-icu" < myvariable;
> ?column?
> ----------
> f
> (1 row)
>
> =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
> ERROR: 42P21: collation mismatch between explicit collations "en-x-icu"
> and "mycollation"
> LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
>
> So it's missing both dependency recording for variable's collation and also
> teaching various code that variables can have a collation.
>
> It's also missing some invalidation detection. For instance:
>
> =# create variable myval text;
> CREATE VARIABLE
>
> =# let myval = 'pg_class';
> LET
>
> =# prepare s(text) as select relname from pg_class where relname = $1 or
> relname = myval;
> PREPARE
>
> =# set plan_cache_mode = force_generic_plan ;
> SET
>
> =# execute s ('');
> relname
> ----------
> pg_class
> (1 row)
>
> =# drop variable myval ;
> DROP VARIABLE
>
> =# create variable myval int;
> CREATE VARIABLE
>
> =# execute s ('');
> ERROR: XX000: cache lookup failed for session variable 16408
>
> The plan should have been discarded and the new plan should fail for type
> problem.
>
> Strangely, subsequent calls don't error out:
>
> =# execute s('');
> relname
> ---------
> (0 rows)
>
> But doing an explain shows that there's a problem:
>
> =# explain execute s('');
> ERROR: XX000: cache lookup failed for variable 16408
>

looks like bug

Regards

Pavel

>
> For example, if I define a variable called "relkind", then psql's \sv
> meta-command is broken because the query it performs can't distinguish
> between the column and the variable.
>
> If variables use : as prefix you´ll never have these conflicts.

select relkind from pg_class where relkind = :relkind

Hi,

On Fri, Jan 14, 2022 at 07:49:09AM -0300, Marcos Pegoraro wrote:
> >
> > For example, if I define a variable called "relkind", then psql's \sv
> > meta-command is broken because the query it performs can't distinguish
> > between the column and the variable.
> >
> If variables use : as prefix you´ll never have these conflicts.
>
> select relkind from pg_class where relkind = :relkind

This is already used by psql client side variables, so this is not an option.

pá 14. 1. 2022 v 11:49 odesílatel Marcos Pegoraro <marcos(at)f10(dot)com(dot)br>
napsal:

> For example, if I define a variable called "relkind", then psql's \sv
>> meta-command is broken because the query it performs can't distinguish
>> between the column and the variable.
>>
>> If variables use : as prefix you´ll never have these conflicts.
>
> select relkind from pg_class where relkind = :relkind
>

This syntax is used for client side variables already.

This is similar to MSSQL or MySQL philosophy. But the disadvantage of this
method is the impossibility of modularization - all variables are in one
space (although there are nested scopes).

The different syntax disallows any collision well, it is far to what is
more usual standard in this area. And if we introduce special syntax, then
there is no way back. We cannot use :varname - this syntax is used already,
but we can use, theoretically, @var or $var. But, personally, I don't want
to use it, if there is possibility to do without it. The special syntax can
be used maybe for direct access to function arguments, or for not
persistent (temporal) session variables like MSSQL. There is a relatively
big space of functionality for session variables, and the system that I
used is based on ANSI SQL/PSM or DB2 and it is near to Oracle. It has a lot
of advantages for writing stored procedures. On other hand, for adhoc work
the session variables like MySQL (without declaration) can be handy, so I
don't want to use (and block) syntax that can be used for something
different.

>
>

Hi

pá 14. 1. 2022 v 3:44 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Thu, Jan 13, 2022 at 07:32:26PM +0100, Pavel Stehule wrote:
> > čt 13. 1. 2022 v 19:23 odesílatel Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com
> >
> > napsal:
> >
> > > On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> > > wrote:
> > > >
> > > > I like the idea of prioritizing tables over variables with warnings
> when
> > > collision is detected. It cannot break anything. And it allows to using
> > > short identifiers when there is not collision.
> > >
> > > Yeah, that seems OK, as long as it's clearly documented. I don't think
> > > a warning is necessary.
>
> What should be the behavior for a cached plan that uses a variable when a
> conflicting relation is later created? I think that it should be the same
> as a
> search_path change and the plan should be discarded.
>
> > The warning can be disabled by default, but I think it should be there.
> > This is a signal, so some in the database schema should be renamed.
> Maybe -
> > session_variables_ambiguity_warning.
>
> I agree that having a way to know that a variable has been bypassed can be
> useful.
>

done

>
> > > (FWIW, testing with dbfiddle, that appears to match Db2's behaviour).
> > >
> >
> > Thank you for check
>
> Do you know what's oracle's behavior on that?
>
>
> I've been looking at the various dependency handling, and I noticed that
> collation are ignored, while they're accepted syntax-wise:
>
> =# "
> CREATE COLLATION
>
> =# create variable myvariable text collate mycollation;
> CREATE VARIABLE
>
> =# select classid::regclass, objid, objsubid, refclassid::regclass,
> refobjid, refobjsubid from pg_depend where classid::regclass::text =
> 'pg_variable' or refclassid::regclass::text = 'pg_variable';
> classid | objid | objsubid | refclassid | refobjid | refobjsubid
> -------------+-------+----------+--------------+----------+-------------
> pg_variable | 16407 | 0 | pg_namespace | 2200 | 0
> (1 row)
>

fixed

>
> =# let myvariable = 'AA';
> LET
>
> =# select 'AA' collate "en-x-icu" < myvariable;
> ?column?
> ----------
> f
> (1 row)
>
> =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
> ERROR: 42P21: collation mismatch between explicit collations "en-x-icu"
> and "mycollation"
> LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
>

What do you expect? I don't understand collating well, but it looks
correct. Minimally the tables have the same behavior.

create collation mycollation (locale = 'fr-FR', provider = 'icu');
create table foo(mycol text collate mycollation);
select 'AA' collate "en-x-icu" < mycol from foo;
┌──────────┐
│ ?column? │
╞══════════╡
│ f │
└──────────┘
(1 row)

postgres=# select 'AA' collate "en-x-icu" < mycol collate mycollation from
foo;
ERROR: collation mismatch between explicit collations "en-x-icu" and
"mycollation"
LINE 1: select 'AA' collate "en-x-icu" < mycol collate mycollation f...
^

> So it's missing both dependency recording for variable's collation and also
> teaching various code that variables can have a collation.
>
> It's also missing some invalidation detection. For instance:
>
> =# create variable myval text;
> CREATE VARIABLE
>
> =# let myval = 'pg_class';
> LET
>
> =# prepare s(text) as select relname from pg_class where relname = $1 or
> relname = myval;
> PREPARE
>
> =# set plan_cache_mode = force_generic_plan ;
> SET
>
> =# execute s ('');
> relname
> ----------
> pg_class
> (1 row)
>
> =# drop variable myval ;
> DROP VARIABLE
>
> =# create variable myval int;
> CREATE VARIABLE
>
> =# execute s ('');
> ERROR: XX000: cache lookup failed for session variable 16408
>
> The plan should have been discarded and the new plan should fail for type
> problem.
>
> Strangely, subsequent calls don't error out:
>
> =# execute s('');
> relname
> ---------
> (0 rows)
>
> But doing an explain shows that there's a problem:
>
> =# explain execute s('');
> ERROR: XX000: cache lookup failed for variable 16408
>

fixed

Please, can you check the attached patches?

Regards

Pavel

=# "
>> CREATE COLLATION
>>
>> =# create variable myvariable text collate mycollation;
>> CREATE VARIABLE
>>
>> =# select classid::regclass, objid, objsubid, refclassid::regclass,
>> refobjid, refobjsubid from pg_depend where classid::regclass::text =
>> 'pg_variable' or refclassid::regclass::text = 'pg_variable';
>> classid | objid | objsubid | refclassid | refobjid | refobjsubid
>> -------------+-------+----------+--------------+----------+-------------
>> pg_variable | 16407 | 0 | pg_namespace | 2200 | 0
>> (1 row)
>>
>
> fixed
>
>
>>
>> =# let myvariable = 'AA';
>> LET
>>
>> =# select 'AA' collate "en-x-icu" < myvariable;
>> ?column?
>> ----------
>> f
>> (1 row)
>>
>> =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
>> ERROR: 42P21: collation mismatch between explicit collations "en-x-icu"
>> and "mycollation"
>> LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
>>
>
> What do you expect? I don't understand collating well, but it looks
> correct. Minimally the tables have the same behavior.
>
> create collation mycollation (locale = 'fr-FR', provider = 'icu');
> create table foo(mycol text collate mycollation);
> select 'AA' collate "en-x-icu" < mycol from foo;
> ┌──────────┐
> │ ?column? │
> ╞══════════╡
> │ f │
> └──────────┘
> (1 row)
>
>
> postgres=# select 'AA' collate "en-x-icu" < mycol collate mycollation from
> foo;
> ERROR: collation mismatch between explicit collations "en-x-icu" and
> "mycollation"
> LINE 1: select 'AA' collate "en-x-icu" < mycol collate mycollation f...
> ^
>
>
here is second test

postgres=# CREATE COLLATION nd2 (
provider = 'icu',
locale = '@colStrength=secondary', -- or 'und-u-ks-level2'
deterministic = false
);
CREATE COLLATION
postgres=# create variable testv as text col

postgres=# create variable testv as text collate nd2;
CREATE VARIABLE
postgres=# let testv = 'Ahoj';
LET
postgres=# select testv = 'AHOJ';
┌──────────┐
│ ?column? │
╞══════════╡
│ t │
└──────────┘
(1 row)

postgres=# select testv = 'AHOJ' collate "default";
┌──────────┐
│ ?column? │
╞══════════╡
│ f │
└──────────┘
(1 row)

Regards

Pavel

>
>

Hi,

On Tue, Jan 18, 2022 at 10:01:01PM +0100, Pavel Stehule wrote:
> pá 14. 1. 2022 v 3:44 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:
>
> >
> > =# let myvariable = 'AA';
> > LET
> >
> > =# select 'AA' collate "en-x-icu" < myvariable;
> > ?column?
> > ----------
> > f
> > (1 row)
> >
> > =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
> > ERROR: 42P21: collation mismatch between explicit collations "en-x-icu"
> > and "mycollation"
> > LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
> >
>
> What do you expect? I don't understand collating well, but it looks
> correct. Minimally the tables have the same behavior.

Indeed, I actually didn't know that such object's collation were implicit and
could be overloaded without a problem as long as there's no conflict between
all the explicit collations. So I agree that the current behavior is ok,
including a correct handling for wanted conflicts:

=# create variable var1 text collate "fr-x-icu";
CREATE VARIABLE

=# create variable var2 text collate "en-x-icu";
CREATE VARIABLE

=# let var1 = 'hoho';
LET

=# let var2 = 'hoho';
LET

=# select var1 < var2;
ERROR: 42P22: could not determine which collation to use for string comparison
HINT: Use the COLLATE clause to set the collation explicitly.

> Please, can you check the attached patches?

All the issue I mentioned are fixed, thanks!

I see a few problems with the other new features added though. The new
session_variables_ambiguity_warning GUC is called even in contexts where it
shouldn't apply. For instance:

=# set session_variables_ambiguity_warning = 1;
SET

=# create variable v text;
CREATE VARIABLE

=# DO $$
DECLARE v text;
BEGIN
v := 'test';
RAISE NOTICE 'v: %', v;
END;
$$ LANGUAGE plpgsql;
WARNING: 42702: session variable "v" is shadowed by column
LINE 1: v := 'test'
^
DETAIL: The identifier can be column reference or session variable reference.
HINT: The column reference is preferred against session variable reference.
QUERY: v := 'test'

But this "v := 'test'" shouldn't be a substitute for a LET, and it indeed
doesn't work:

=# DO $$
BEGIN
v := 'test';
RAISE NOTICE 'v: %', v;
END;
$$ LANGUAGE plpgsql;
ERROR: 42601: "v" is not a known variable
LINE 3: v := 'test';

But the RAISE NOTICE does see the session variable (which should be the correct
behavior I think), so the warning should have been raised for this instruction
(and in that case the message is incorrect, as it's not shadowing a column).

Also, the pg_dump handling emits a COLLATION option for session variables even
for default collation, while it should only emit it if the collation is not the
type's default collation. As a reference, for attributes the SQL used is:

"CASE WHEN a.attcollation <> t.typcollation "
"THEN a.attcollation ELSE 0 END AS attcollation,\n"

Also, should \dV or \dV+ show the collation?

And a few comments on the new chunks in this version of the patch (I didn't
look in detail at the whole patch yet):

+ <para>
+ The session variables can be overshadowed by columns in an query. When query
+ holds identifier or qualified identifier that can be used as session variable
+ identifier and as column identifier too, then it is used as column identifier
+ every time. This situation can be logged by enabling configuration
+ parameter <xref linkend="guc-session-variables-ambiguity-warning"/>.
+ </para>

Is "overshadowed" correct? The rest of the patch only says "shadow(ed)".

While at it, here's some proposition to improve the phrasing:

+ 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 <xref
+ linkend="guc-session-variables-ambiguity-warning"/>.

Similarly, the next documentation could be rephrased to:

+ When on, a warning is raised when any identifier in a query could be used as both
+ a column identifier or a session variable identifier.
+ The default is <literal>off</literal>.

Few other nitpicking:

+ * If we really detect collision of column and variable identifier,
+ * then we prefer column, because we don't want to allow to break
+ * an existing valid queries by new variable.

s/an existing/existing

+-- it is ambigonuous, but columns are preferred

ambiguous?

@@ -369,6 +367,19 @@ VariableCreate(const char *varName,
/* dependency on extension */
recordDependencyOnCurrentExtension(&myself, false);

+ /*
+ * Normal dependency from a domain to its collation. We know the default
+ * collation is pinned, so don't bother recording it.
+ */
+ if (OidIsValid(varCollation) &&
+ varCollation != DEFAULT_COLLATION_OID)

The comment mentions domains rather than session variables.

And for the initial patch, while looking around I found this comment on
fix_alternative_subplan():

@@ -1866,7 +1969,9 @@ fix_alternative_subplan(PlannerInfo *root, AlternativeSubPlan *asplan,
* replacing Aggref nodes that should be replaced by initplan output Params,
* choosing the best implementation for AlternativeSubPlans,
* looking up operator opcode info for OpExpr and related nodes,
- * and adding OIDs from regclass Const nodes into root->glob->relationOids.
+ * and adding OIDs from regclass Const nodes into root->glob->relationOids,
+ * and replacing PARAM_VARIABLE paramid, that is the oid of the session variable
+ * to offset the array by query used session variables. ???

I don't really understand the comment, and the "???" looks a bit suspicious.
I'm assuming it's a reference to this new behavior in fix_param_node():

* fix_param_node
* Do set_plan_references processing on a Param
+ * Collect session variables list and replace variable oid by
+ * index to collected list.
*
* If it's a PARAM_MULTIEXPR, replace it with the appropriate Param from
* root->multiexpr_params; otherwise no change is needed.
* Just for paranoia's sake, we make a copy of the node in either case.
+ *
+ * If it's a PARAM_VARIABLE, then we should to calculate paramid.

Some improvement on the comments would be welcome there, probably including
some mention to the "glob->sessionVariables" collected list?

st 19. 1. 2022 v 9:01 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> Hi,
>
> On Tue, Jan 18, 2022 at 10:01:01PM +0100, Pavel Stehule wrote:
> > pá 14. 1. 2022 v 3:44 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com>
> napsal:
> >
> > >
> > > =# let myvariable = 'AA';
> > > LET
> > >
> > > =# select 'AA' collate "en-x-icu" < myvariable;
> > > ?column?
> > > ----------
> > > f
> > > (1 row)
> > >
> > > =# select 'AA' collate "en-x-icu" < myvariable collate mycollation;
> > > ERROR: 42P21: collation mismatch between explicit collations
> "en-x-icu"
> > > and "mycollation"
> > > LINE 1: select 'AA' collate "en-x-icu" < myvariable collate mycollat...
> > >
> >
> > What do you expect? I don't understand collating well, but it looks
> > correct. Minimally the tables have the same behavior.
>
> Indeed, I actually didn't know that such object's collation were implicit
> and
> could be overloaded without a problem as long as there's no conflict
> between
> all the explicit collations. So I agree that the current behavior is ok,
> including a correct handling for wanted conflicts:
>
> =# create variable var1 text collate "fr-x-icu";
> CREATE VARIABLE
>
> =# create variable var2 text collate "en-x-icu";
> CREATE VARIABLE
>
> =# let var1 = 'hoho';
> LET
>
> =# let var2 = 'hoho';
> LET
>
> =# select var1 < var2;
> ERROR: 42P22: could not determine which collation to use for string
> comparison
> HINT: Use the COLLATE clause to set the collation explicitly.
>
> > Please, can you check the attached patches?
>
> All the issue I mentioned are fixed, thanks!
>
>
thank you for check

>
> I see a few problems with the other new features added though. The new
> session_variables_ambiguity_warning GUC is called even in contexts where it
> shouldn't apply. For instance:
>
> =# set session_variables_ambiguity_warning = 1;
> SET
>
> =# create variable v text;
> CREATE VARIABLE
>
> =# DO $$
> DECLARE v text;
> BEGIN
> v := 'test';
> RAISE NOTICE 'v: %', v;
> END;
> $$ LANGUAGE plpgsql;
> WARNING: 42702: session variable "v" is shadowed by column
> LINE 1: v := 'test'
> ^
> DETAIL: The identifier can be column reference or session variable
> reference.
> HINT: The column reference is preferred against session variable
> reference.
> QUERY: v := 'test'
>
> But this "v := 'test'" shouldn't be a substitute for a LET, and it indeed
> doesn't work:
>

Yes, there are some mistakes (bugs). The PLpgSQL assignment as target
should not see session variables, so warning is nonsense there. RAISE
NOTICE should use local variables, and in this case is a question if we
should raise a warning. There are two possible analogies - we can see
session variables like global variables, and then the warning should not be
raised, or we can see relation between session variables and plpgsql
variables similar like session variables and some with higher priority, and
then warning should be raised. If we want to ensure that the new session
variable doesn't break code, then session variables should have lower
priority than plpgsql variables too. And because the plpgsql protection
against collision cannot be used, then I prefer raising the warning.

PLpgSQL assignment should not be in collision with session variables ever

>
> =# DO $$
> BEGIN
> v := 'test';
> RAISE NOTICE 'v: %', v;
> END;
> $$ LANGUAGE plpgsql;
> ERROR: 42601: "v" is not a known variable
> LINE 3: v := 'test';
>
> But the RAISE NOTICE does see the session variable (which should be the
> correct
> behavior I think), so the warning should have been raised for this
> instruction
> (and in that case the message is incorrect, as it's not shadowing a
> column).
>

In this case I can detect node type, and I can identify external param
node, but I cannot to detect if this code was executed from PLpgSQL or from
some other

So I can to modify warning text to some

DETAIL: The identifier can be column reference or query parameter or
session variable reference.
HINT: The column reference and query parameter is preferred against
session variable reference.

I cannot to use term "plpgsql variable" becase I cannot to ensure validity
of this message

Maybe is better to don't talk about source of this issue, and just talk
about result - so the warning text should be just

MESSAGE: "session variable \"xxxx\" is shadowed
DETAIL: "session variables can be shadowed by columns, routine's variables
and routine's arguments with same name"

Is it better?

Hi,

On Wed, Jan 19, 2022 at 09:09:41PM +0100, Pavel Stehule wrote:
> st 19. 1. 2022 v 9:01 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:
>
> RAISE NOTICE should use local variables, and in this case is a question if we
> should raise a warning. There are two possible analogies - we can see session
> variables like global variables, and then the warning should not be raised,
> or we can see relation between session variables and plpgsql variables
> similar like session variables and some with higher priority, and then
> warning should be raised. If we want to ensure that the new session variable
> doesn't break code, then session variables should have lower priority than
> plpgsql variables too. And because the plpgsql protection against collision
> cannot be used, then I prefer raising the warning.

Ah that's indeed a good point. I agree, they're from a different part of the
system so they should be treated as different things, and thus raising a
warning. It's consistent with the chosen conservative approach anyway.

> PLpgSQL assignment should not be in collision with session variables ever

Agreed.

>
> >
> > =# DO $$
> > BEGIN
> > v := 'test';
> > RAISE NOTICE 'v: %', v;
> > END;
> > $$ LANGUAGE plpgsql;
> > ERROR: 42601: "v" is not a known variable
> > LINE 3: v := 'test';
> >
> > But the RAISE NOTICE does see the session variable (which should be the
> > correct
> > behavior I think), so the warning should have been raised for this
> > instruction
> > (and in that case the message is incorrect, as it's not shadowing a
> > column).
> >
>
> In this case I can detect node type, and I can identify external param
> node, but I cannot to detect if this code was executed from PLpgSQL or from
> some other
>
> So I can to modify warning text to some

Yes, that's what I had in mind too.

> DETAIL: The identifier can be column reference or query parameter or
> session variable reference.
> HINT: The column reference and query parameter is preferred against
> session variable reference.
>
> I cannot to use term "plpgsql variable" becase I cannot to ensure validity
> of this message
>
> Maybe is better to don't talk about source of this issue, and just talk
> about result - so the warning text should be just
>
> MESSAGE: "session variable \"xxxx\" is shadowed
> DETAIL: "session variables can be shadowed by columns, routine's variables
> and routine's arguments with same name"
>
> Is it better?

I clearly prefer the 2nd version.