Обсуждение: Schema variables - new implementation for Postgres 15

From: Pavel Stehule <pavel.stehule@gmail.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

 

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

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

Hi

only rebase

Regards

Pavel

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

Hi

only rebase

second try - rebase after serial_scheduler remove

Regards

Pavel

Regards

Pavel

Hi

rebase

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/

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/

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/

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

Le 09/09/2021 à 11:40, Pavel Stehule a écrit :

Hi

čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er@xs4all.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/

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

On 9/10/21 10:06 AM, Pavel Stehule wrote:
> Hi
>
> čt 9. 9. 2021 v 12:21 odesílatel Erik Rijkers <er@xs4all.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
>
>
>>
>

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

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

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

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.vondra@enterprisedb.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

On 11/6/21 04:45, Pavel Stehule wrote:
> Hi
>
> st 3. 11. 2021 v 14:05 odesílatel Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.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

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

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

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/20211106013904.GG17618@telsasoft.com

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

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/20211106013904.GG17618@telsasoft.com

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

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@xs4all.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

Op 19-12-2021 om 08:13 schreef Pavel Stehule:
>
>
> ne 19. 12. 2021 v 8:09 odesílatel Erik Rijkers <er@xs4all.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

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

On Wed, 3 Nov 2021 at 13:05, Tomas Vondra <tomas.vondra@enterprisedb.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

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

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

On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel.stehule@gmail.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

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.a.rasheed@gmail.com>
> napsal:
>
> > On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel.stehule@gmail.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

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.

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 Thu, Jan 13, 2022 at 07:32:26PM +0100, Pavel Stehule wrote:
> čt 13. 1. 2022 v 19:23 odesílatel Dean Rasheed <dean.a.rasheed@gmail.com>
> napsal:
>
> > On Thu, 13 Jan 2022 at 17:42, Pavel Stehule <pavel.stehule@gmail.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

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

=# "
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...
                                               ^

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@gmail.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).

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@gmail.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?

Hi,

On Fri, Jan 21, 2022 at 09:23:34PM +0100, Pavel Stehule wrote:
>
> I am sending updated patches

I've been looking a bit deeper at the feature and I noticed that there's no
locking involved around the session variable usage, and I don't think that's
ok.  AFAICS any variable used in a session will be cached in the local hash
table and will never try to access some catalog or cache, so I don't have any
naive scenario that would immediately crash, but this has some other
implications that seems debatable.

For instance, right now nothing prevents a variable from being dropped while
another session is using it.

Obviously we can't lock a session variable forever just because a session
assigned a value once ages ago, especially outside of the current transaction.
But if a session set a variable in the local transaction, I don't think that
it's ok to have a subsequent query failing because someone else concurrently
dropped the variable.

I only backlogged this current thread but I didn't see that being discussed.

Hi,

On Sun, Jan 23, 2022 at 09:25:56AM +0100, Pavel Stehule wrote:
> ne 23. 1. 2022 v 9:10 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
>
> Isn't there enough stability of the system cache? sinval is sent at the
> moment when changes in the system catalog are visible. So inside query
> execution I don't see that the variable was dropped in another session.

Yes, inside a single query it should probably be ok, but I'm talking about
multiple query execution in the same transaction.

I tested it now. a sinval message is waiting on the transaction end.  So when a variable is used, then it is working fine until the transaction ends.

But when the session makes some DDL, then send sinval to self, and at this moment, the variable can be dropped before the transaction ends.

Hi, 

Le dim. 23 janv. 2022 à 22:34, Pavel Stehule <pavel.stehule@gmail.com> a écrit :

I tested it now. a sinval message is waiting on the transaction end.  So when a variable is used, then it is working fine until the transaction ends.

But when the session makes some DDL, then send sinval to self, and at this moment, the variable can be dropped before the transaction ends.

a backend can accept sinval in very common scenarios, like acquiring a heavyweight lock. That includes accessing a relation thats not in the catcache, so that's really critical to have a protection here. 

Hi,

On Mon, Jan 24, 2022 at 12:33:11PM +0100, Pavel Stehule wrote:
>
> here is updated patch with locking support

Thanks for updating the patch!

While the locking is globally working as intended, I found a few problems with
it.

First, I don't think that acquiring the lock in
get_session_variable_type_typmod_collid() and prepare_variable_for_reading() is
the correct approach.  In transformColumnRef() and transformLetStmt() you first
call IdentifyVariable() to check if the given name is a variable without
locking it and later try to lock the variable if you get a valid Oid.  This is
bug prone as any other backend could drop the variable between the two calls
and you would end up with a cache lookup failure.  I think the lock should be
acquired during IdentifyVariable.  It should probably be optional as one
codepath only needs the information to raise a warning when a variable is
shadowed, so a concurrent drop isn't a problem there.

Hi,

On Tue, Jan 25, 2022 at 09:35:09AM +0100, Pavel Stehule wrote:
> út 25. 1. 2022 v 6:18 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
>
> > I think the lock should be
> > acquired during IdentifyVariable.  It should probably be optional as one
> > codepath only needs the information to raise a warning when a variable is
> > shadowed, so a concurrent drop isn't a problem there.
> >
>
> There is a problem, because before the IdentifyVariable call I don't know
> if the variable will be shadowed or not.
>
> If I lock a variable inside IdentifyVariable, then I need to remember if I
> did lock there, or if the variable was locked already, and If the variable
> is shadowed and if lock is fresh, then I can unlock the variable.

But in transformColumnRef() you already know if you found a matching column or
not when calling IdentifyVariable(), so you know if an existing variable will
shadow it right?

Couldn't you call something like

    lockit = node == NULL;
        varid = IdentifyVariable(cref->fields, &attrname, &not_unique, lockit);

The only other caller is transformLetStmt(), which should always lock the
variable anyway.

Hi,

On Mon, Jan 24, 2022 at 12:33:11PM +0100, Pavel Stehule wrote:
>
> here is updated patch with locking support

Thanks for updating the patch!

While the locking is globally working as intended, I found a few problems with
it.

First, I don't think that acquiring the lock in
get_session_variable_type_typmod_collid() and prepare_variable_for_reading() is
the correct approach.  In transformColumnRef() and transformLetStmt() you first
call IdentifyVariable() to check if the given name is a variable without
locking it and later try to lock the variable if you get a valid Oid.  This is
bug prone as any other backend could drop the variable between the two calls
and you would end up with a cache lookup failure.  I think the lock should be
acquired during IdentifyVariable.  It should probably be optional as one
codepath only needs the information to raise a warning when a variable is
shadowed, so a concurrent drop isn't a problem there.

For prepare_variable_for_reading(), the callers are CopySessionVariable() and
GetSessionVariable().  IIUC those should take care of executor-time locks, but
shouldn't there be some changes for planning, like in AcquirePlannerLocks()?

Some other comments on this part of the patch:

@@ -717,6 +730,9 @@ RemoveSessionVariable(Oid varid)
    Relation    rel;
    HeapTuple   tup;

+   /* Wait, when dropped variable is not used */
+   LockDatabaseObject(VariableRelationId, varid, 0, AccessExclusiveLock);

Why do you explicitly try to acquire an AEL on the variable here?
RemoveObjects / get_object_address should guarantee that this was already done.
You could add an assert LockHeldByMe() here, but no other code path do it so it
would probably waste cycles in assert builds for nothing as it's a fundamental
guarantee.

@@ -747,6 +763,9 @@ RemoveSessionVariable(Oid varid)
     * only when current transaction will be commited.
     */
    register_session_variable_xact_action(varid, ON_COMMIT_RESET);
+
+   /* Release lock */
+   UnlockDatabaseObject(VariableRelationId, varid, 0, AccessExclusiveLock);
 }

Why releasing the lock here?  It will be done at the end of the transaction,
and you certainly don't want other backends to start using this variable in
between.  Also, since you acquired the lock a second time it only decreases the
lock count in the locallock so the lock isn't released anyway.

+ * Returns type, typmod and collid of session variable.
+ *
+ * As a side effect this function acquires AccessShareLock on the
+ * related session variable.
  */
 void
-get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod, Oid *collid)
+get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod, Oid *collid,
+                                       bool lock_held)


lock_held is a bit misleading.  If you keep some similar parameter for this or
another function, maybe name it lock_it or something like that instead?

Also, the comment isn't accurate and should say that an ASL is acquired iff the
variable is true.

+   /*
+    * Acquire a lock on session variable, which we won't release until commit.
+    * This ensure that one backend cannot to drop session variable used by
+    * second backend.
+    */

(and similar comments)
I don't think it's necessary to explain why we acquire locks, we should just
say that the lock will be kept for the whole transaction (and not until a
commit)

And while looking at nearby code, it's probably worthwhile to add an Assert in
create_sessionvars_hashtable() to validate that sessionvars htab is NULL.

sessionvariable.c:

+ * Although session variables are not transactional, we don't
+ * want (and we cannot) to run cleaning immediately (when we
+ * got sinval message). The value of session variables can
+ * be still used or the operation that emits cleaning can be
+ * reverted. Unfortunatelly, this check can be done only in
+ * when transaction is committed (the check against system
+ * catalog requires transaction state).

This was the original idea, but since there's now locking to make all DDL safe,
the metadata should be considered fully transactional and no session should
still be able to use a concurrently dropped variable.  Also, the invalidation
messages are not sent until the transaction is committed.  So is that approach
still needed (at least for things outside ON COMMIT DROP / ON TRANSACTION END
RESET

Hi,

On Tue, Jan 25, 2022 at 10:53:00PM +0100, Pavel Stehule wrote:
>
> út 25. 1. 2022 v 6:18 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
> >
> > First, I don't think that acquiring the lock in
> > get_session_variable_type_typmod_collid() and
> > prepare_variable_for_reading() is
> > the correct approach.  In transformColumnRef() and transformLetStmt() you
> > first
> > call IdentifyVariable() to check if the given name is a variable without
> > locking it and later try to lock the variable if you get a valid Oid.
> > This is
> > bug prone as any other backend could drop the variable between the two
> > calls
> > and you would end up with a cache lookup failure.  I think the lock should
> > be
> > acquired during IdentifyVariable.  It should probably be optional as one
> > codepath only needs the information to raise a warning when a variable is
> > shadowed, so a concurrent drop isn't a problem there.
> >
>
> I moved lock to IdentifyVariable routine

+IdentifyVariable(List *names, char **attrname, bool lockit, bool *not_unique)
+{
[...]
+               return varoid_without_attr;
+           }
+           else
+           {
+               *attrname = c;
+               return varoid_with_attr;
[...]
+
+   if (OidIsValid(varid) && lockit)
+       LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
+
+   return varid;

There are still some code paths that may not lock the target variable when
required.

Also, the function comment doesn't say much about attrname handling, it should
be clarifed.  I think it should initially be set to NULL, to make sure that
it's always a valid pointer after the function returns.

> attached updated patch

Various comments on the patch:

No test for GRANT/REVOKE ... ALL VARIABLES IN SCHEMA, maybe it would be good to
have one?

Documentation:

catalogs.sgml:

You're still using the old-style 4 columns table, it should be a single column
like the rest of the file.

+  <para>
+   The <command>CREATE VARIABLE</command> command creates a session variable.
+   Session variables, like relations, exist within a schema and their access is
+   controlled via <command>GRANT</command> and <command>REVOKE</command>
+   commands.  Changing a session variable is non-transactional.
+  </para>

The "changing a session variable is non-transactional" is ambiguous.  I think
that only the value part isn't transactional, the variable metadata themselves
(ALTER VARIABLE and other DDL) are transactional right?  This should be
explicitly described here (although it's made less ambiguous in the next
paragraph).

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

should this part mention that session variables can be shadowed?  For now the
only mention to that is in advanced.sgml.

+      The <literal>DEFAULT</literal> clause can be used to assign a default
+      value to a session variable.

The expression is lazily evaluated during the session first use of the
variable.  This should be documented as any usage of volatile expression will
be impacted.

+      The <literal>ON TRANSACTION END RESET</literal>
+      clause causes the session variable to be reset to its default value when
+      the transaction is committed or rolled back.

As far as I can see this clauses doesn't play well with IMMUTABLE VARIABLE, as
you can reassign a value once the transaction ends.  Same for DISCARD [ ALL |
VARIABLES ], or LET var = NULL (or DEFAULT if no default value).  Is that
intended?

+   <literal>LET</literal> extends the syntax defined in the SQL
+   standard. The <literal>SET</literal> command from the SQL standard
+   is used for different purposes in <productname>PostgreSQL</productname>.

I don't fully understand that.  Are (session) variables defined in the SQL
standard?  If yes, all the other documentation pages should clarify that as
they currently say that this is a postgres extension.  If not, this part should
made it clear what is defined in the standard.

In revoke.sgml:
+ REVOKE [ GRANT OPTION FOR ]
+     { { READ | WRITE } [, ...] | ALL [ PRIVILEGES ] }
+     ON VARIABLE <replaceable>variable_name</replaceable> [, ...]
+     FROM { [ GROUP ] <replaceable class="parameter">role_name</replaceable> | PUBLIC } [, ...]
+     [ CASCADE | RESTRICT ]

there's no extra documentation for that, and therefore no clarification on
variable_name.

VariableIsVisible():
+                * If it is in the path, it might still not be visible; it could be
+                * hidden by another relation of the same name earlier in the path. So
+                * we must do a slow check for conflicting relations.

should it be "another variable of the same name"?

Tab completion: CREATE IMMUTABLE VARIABLE is not handled

pg_variable.c:
Do we really need both session_variable_get_name() and
get_session_variable_name()?

+/*
+ * Fetch all fields of session variable from the syscache.
+ */
+void
+initVariable(Variable *var, Oid varid, bool missing_ok, bool fast_only)

As least fast_only should be documented in the function comment, especially
regarding var->varname, since:

+   var->oid = varid;
+   var->name = pstrdup(NameStr(varform->varname));
[...]
+   if (!fast_only)
+   {
+       Datum       aclDatum;
+       bool        isnull;
+
+       /* name */
+       var->name = pstrdup(NameStr(varform->varname));A
[...]
+   else
+   {
+       var->name = NULL;

is the output value guaranteed or not?  In any case it shouldn't be set twice.

Also, I don't see any caller for missing_ok == true, should we remove it?

+/*
+ * Create entry in pg_variable table
+ */
+ObjectAddress
+VariableCreate(const char *varName,
[...]
+   /* dependency on any roles mentioned in ACL */
+   if (varacl != NULL)
+   {
+       int         nnewmembers;
+       Oid        *newmembers;
+
+       nnewmembers = aclmembers(varacl, &newmembers);
+       updateAclDependencies(VariableRelationId, varid, 0,
+                             varOwner,
+                             0, NULL,
+                             nnewmembers, newmembers);

Shouldn't you use recordDependencyOnNewAcl() instead?  Also, sn't it missing a
recordDependencyOnOwner()?

sessionvariable.c:

+ * Although session variables are not transactional, we don't
+ * want (and we cannot) to run cleaning immediately (when we
+ * got sinval message). The value of session variables can
+ * be still used or the operation that emits cleaning can be
+ * reverted. Unfortunatelly, this check can be done only in
+ * when transaction is committed (the check against system
+ * catalog requires transaction state).

This was the original idea, but since there's now locking to make all DDL safe,
the metadata should be considered fully transactional and no session should
still be able to use a concurrently dropped variable.  Also, the invalidation
messages are not sent until the transaction is committed.  So is that approach
still needed (at least for things outside ON COMMIT DROP / ON TRANSACTION END
RESET)?

I'm also attaching a 3rd patch with some proposition for documentation
rewording (including consistent use of *session* variable), a few comments
rewording, copyright year bump and minor things like that.

Note that I still didn't really review pg_variable.c or sessionvariable.c since
there might be significant changes there for either the sinval / immutable part
I mentioned.

Hi,

On Fri, Jan 28, 2022 at 07:51:08AM +0100, Pavel Stehule wrote:
> st 26. 1. 2022 v 8:23 odesílatel Julien Rouhaud <rjuju123@gmail.com> napsal:
>
> > +      The <literal>ON TRANSACTION END RESET</literal>
> > +      clause causes the session variable to be reset to its default value
> > when
> > +      the transaction is committed or rolled back.
> >
> > As far as I can see this clauses doesn't play well with IMMUTABLE
> > VARIABLE, as
> > you can reassign a value once the transaction ends.  Same for DISCARD [
> > ALL |
> > VARIABLES ], or LET var = NULL (or DEFAULT if no default value).  Is that
> > intended?
> >
>
> I think so it is expected. The life scope of assigned (immutable) value is
> limited to transaction (when ON TRANSACTION END RESET).
> DISCARD is used for reset of session, and after it, you can write the value
> first time.
>
> I enhanced doc in IMMUTABLE clause

I think it's still somewhat unclear:

-      done, no other change will be allowed in the session lifetime.
+      done, no other change will be allowed in the session variable content's
+      lifetime. The lifetime of content of session variable can be
+      controlled by <literal>ON TRANSACTION END RESET</literal> clause.
+     </para>

The "session variable content lifetime" is quite peculiar, as the ON
TRANSACTION END RESET is adding transactional behavior to something that's not
supposed to be transactional, so more documentation about it seems appropriate.

Also DISCARD can be used any time so that's a totally different aspect of the
immutable variable content lifetime that's not described here.

NULL handling also seems inconsistent.  An explicit default NULL value makes it
truly immutable, but manually assigning NULL is a different codepath that has a
different user behavior:

# create immutable variable var_immu int default null;
CREATE VARIABLE

# let var_immu = 1;
ERROR:  22005: session variable "var_immu" is declared IMMUTABLE

# create immutable variable var_immu2 int ;
CREATE VARIABLE

# let var_immu2 = null;
LET

# let var_immu2 = null;
LET

# let var_immu2 = 1;
LET

For var_immu2 I think that the last 2 queries should have errored out.

> > In revoke.sgml:
> > + REVOKE [ GRANT OPTION FOR ]
> > +     { { READ | WRITE } [, ...] | ALL [ PRIVILEGES ] }
> > +     ON VARIABLE <replaceable>variable_name</replaceable> [, ...]
> > +     FROM { [ GROUP ] <replaceable
> > class="parameter">role_name</replaceable> | PUBLIC } [, ...]
> > +     [ CASCADE | RESTRICT ]
> >
> > there's no extra documentation for that, and therefore no clarification on
> > variable_name.
> >
>
> This is same like function_name, domain_name, ...

Ah right.

> > pg_variable.c:
> > Do we really need both session_variable_get_name() and
> > get_session_variable_name()?
> >
>
> They are different - first returns possibly qualified name, second returns
> only name. Currently it is used just for error messages in
> transformAssignmentIndirection, and I think so it is good for consistency
> with other usage of this routine (transformAssignmentIndirection).

I agree that consistency with other usage is a good thing, but both functions
have very similar and confusing names.  Usually when you need the qualified
name the calling code just takes care of doing so.  Wouldn't it be better to
add say get_session_variable_namespace() and construct the target string in the
calling code?

Also, I didn't dig a lot but I didn't see other usage with optionally qualified
name there?  I'm not sure how it would make sense anyway since LET semantics
are different and the current call for session variable emit incorrect
messages:

# create table tt(id integer);
CREATE TABLE

# create variable vv tt;
CREATE VARIABLE

# let vv.meh = 1;
ERROR:  42703: cannot assign to field "meh" of column "meh" because there is no such column in data type tt
LINE 1: let vv.meh = 1;

Hi,

On Sun, Jan 30, 2022 at 08:09:18PM +0100, Pavel Stehule wrote:
>
> rebase after 02b8048ba5dc36238f3e7c3c58c5946220298d71

Here are a few comments, mostly about pg_variable.c and sessionvariable.c.  I
stopped before reading the whole patch as I have some concern about the sinval
machanism, which ould change a bit the rest of the patch.  I'm also attaching a
patch (with .txt extension to avoid problem with the cfbot) with some comment
update propositions.

In sessionvariable.c, why VariableEOXAction and VariableEOXActionCodes?  Can't
the parser emit directly the char value, like e.g. relpersistence?

extraneous returns for 2 functions:

+void
+get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod,
+                                       Oid *collid)
+{
[...]
+   return;
+}

+void
+initVariable(Variable *var, Oid varid, bool fast_only)
+{
[...]
+   return;
+}

VariableCreate():

Maybe add a bunch of AssertArg() for all the mandatory parametrers?

Also, the check for variable already existing should be right after the
AssertArg(), and using SearchSysCacheExistsX().

Maybe also adding an Assert(OidIsValid(xxxoid)) just after the
CatalogTupleInsert(), similarly to some other creation functions?

event-triggers.sgml needs updating for the firing matrix, as session variable
are compatible with even triggers.

+typedef enum SVariableXActAction
+{
+   ON_COMMIT_DROP,     /* used for ON COMMIT DROP */
+   ON_COMMIT_RESET,    /* used for DROP VARIABLE */
+   RESET,              /* used for ON TRANSACTION END RESET */
+   RECHECK             /* recheck if session variable is living */
+} SVariableXActAction;

The names seem a bit generic, maybe add a prefix like SVAR_xxx?

ON_COMMIT_RESET is also confusing as it looks like an SQL clause.  Maybe
PERFORM_DROP or something?

+static List *xact_drop_actions = NIL;
+static List *xact_reset_actions = NIL;

Maybe add a comment saying both are lists of SVariableXActAction?

+typedef SVariableData * SVariable;

looks like a missing bump to typedefs.list.

+char *
+get_session_variable_name(Oid varid)
+{
+   HeapTuple   tup;
+   Form_pg_variable varform;
+   char       *varname;
+
+   tup = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(varid));
+
+   if (!HeapTupleIsValid(tup))
+       elog(ERROR, "cache lookup failed for session variable %u", varid);
+
+   varform = (Form_pg_variable) GETSTRUCT(tup);
+
+   varname = NameStr(varform->varname);
+
+   ReleaseSysCache(tup);
+
+   return varname;
+}

This kind of function should return a palloc'd copy of the name.

+void
+ResetSessionVariables(void)
[...]
+   list_free_deep(xact_drop_actions);
+   xact_drop_actions = NIL;
+
+   list_free_deep(xact_reset_actions);
+   xact_drop_actions = NIL;
+}

The 2nd chunk should be xact_reset_actions = NIL

+static void register_session_variable_xact_action(Oid varid, SVariableXActAction action);
+static void delete_session_variable_xact_action(Oid varid, SVariableXActAction action);

The naming is a bit confusing, maybe unregister_session_cable_xact_action() for
consistency?

+void
+register_session_variable_xact_action(Oid varid,
+                                     SVariableXActAction action)

the function is missing the static keyword.

In AtPreEOXact_SessionVariable_on_xact_actions(), those 2 instructions are
executed twice (once in the middle and once at the end):

        list_free_deep(xact_drop_actions);
        xact_drop_actions = NIL;

+    * If this entry was created during the current transaction,
+    * creating_subid is the ID of the creating subxact; if created in a prior
+    * transaction, creating_subid is zero.

I don't see any place in the code where creating_subid can be zero? It looks
like it's only there for future transactional implementation, but for now this
attribute seems unnecessary?

                /* at transaction end recheck sinvalidated variables */
                RegisterXactCallback(sync_sessionvars_xact_callback, NULL);

I don't think it's ok to use xact callback for in-core code.  The function
explicitly says:

> * These functions are intended for use by dynamically loaded modules.
> * For built-in modules we generally just hardwire the appropriate calls
> * (mainly because it's easier to control the order that way, where needed).