Обсуждение: Re: proposal: schema variables

Hi folks,

Thanks for continuing this work. As a side note, I would like to mention
how strange the situation in this CF item is. If I understand correctly,
there are two hackers threads discussing the same patch, with recent
patches posted in both of them. This is obviously confusing, e.g. the
main concern from another thread, about names clashing, wasn't even
mentioned in this one. Is it possible to reconcile development in one
thread?

> On Sun, Nov 10, 2024 at 05:19:09PM GMT, Pavel Stehule wrote:
> ne 10. 11. 2024 v 16:24 odesílatel Dmitry Dolgov <9erthalion6@gmail.com>
> napsal:
>
> > Hi folks,
> >
> > Thanks for continuing this work. As a side note, I would like to mention
> > how strange the situation in this CF item is. If I understand correctly,
> > there are two hackers threads discussing the same patch, with recent
> > patches posted in both of them. This is obviously confusing, e.g. the
> > main concern from another thread, about names clashing, wasn't even
> > mentioned in this one. Is it possible to reconcile development in one
> > thread?
> >
>
> This is probably my error. I don't try to organize threads, just I'll try
> to reply in the thread where I got a question.

It's fine. I just would appreciate some clarity about which patch to
look at. To confirm, the series in this thread contains everything from
the other one, plus some improvements, right?

> On Sun, Nov 10, 2024 at 06:51:40PM GMT, Pavel Stehule wrote:
> ne 10. 11. 2024 v 17:19 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
> napsal:
> I thought a lot of time about better solutions for identifier collisions
> and I really don't think so there is some consistent user friendly syntax.
> Personally I think there is an easy already implemented solution -
> convention - just use a dedicated schema for variables and this schema
> should not be in the search path. Or use secondary convention - like using
> prefix "__" for session variables. Common convention is using "_" for
> PLpgSQL variables. I searched how this issue is solved in other databases,
> or in standard, and I found nothing special. The Oracle and SQL/PSM has a
> concept of visibility - the variables are not visible outside packages or
> modules, but Postgres has nothing similar. It can be emulated by a
> dedicated schema without inserting a search path, but it is less strong.
>
> I think we can introduce an alternative syntax, that will not be user
> friendly or readable friendly, but it can be without collisions - or can
> decrease possible risks.
>
> It is nothing new - SQL does it with old, "new" syntax of inner joins, or
> in Postgres we can
>
> where salary < 40000
>
> or
>
> where pg_catalog.int4lt(salary, 40000);
>
>
> or some like we use for operators OPERATOR(*schema*.*operatorname*)
>
> So introducing VARIABLE(schema.variablename) syntax as an alternative
> syntax for accessing variables I really like. I strongly prefer to use this
> as only alternative (secondary) syntax, because I don't think it is
> friendly syntax or writing friendly, but it is safe, and I can imagine
> tools that can replace generic syntax to this special, or that detects
> generic syntax and shows some warning. Then users can choose what they
> prefer. Two syntaxes - generic and special can be good enough for all - and
> this can be perfectly consistent with current Postgres.

As far as I recall, last time this topic was discussed in hackers, two
options were proposed: the one with VARIABLE(name), what you mention
here; and another one with adding variables to the FROM clause. The
VARIABLE(...) syntax didn't get much negative feedback, so I guess why
not -- if you find it fitting, it would be interesting to see the
implementation.

I'm afraid it should not be just an alternative syntax, but the only one
allowed, because otherwise I don't see how scenarious like "drop a
column with the same name" could be avoided. As in the previous thread:

    -- we've got a variable b at the same time
    SELECT a, b FROM table1;

Then dropping the column b, but everything still works beause the
variable b got silently picked up. But if it would be required to say
VARIABLE(b), then all fine.

And to make sure we're on the same page, could you post couple of
examples from curretly existing tests in the patch, how are they going
to look like with this proposal?

About adding variables to the FROM clause. Looks like this option was
quite popular, and you've mentioned some technical challenges
implementing that. If you'd like to go with another approach, it would
be great to elaborate on that -- maybe even with a PoC, to make a
convincing point here.

> On Sat, Nov 16, 2024 at 07:10:31AM GMT, Pavel Stehule wrote:

Sorry, got distracted. Let me try to answer step by step.

> > As far as I recall, last time this topic was discussed in hackers, two
> > options were proposed: the one with VARIABLE(name), what you mention
> > here; and another one with adding variables to the FROM clause. The
> > VARIABLE(...) syntax didn't get much negative feedback, so I guess why
> > not -- if you find it fitting, it would be interesting to see the
> > implementation.
> >
> > I'm afraid it should not be just an alternative syntax, but the only one
> > allowed, because otherwise I don't see how scenarious like "drop a
> > column with the same name" could be avoided. As in the previous thread:
> >
> >     -- we've got a variable b at the same time
> >     SELECT a, b FROM table1;
> >
>
> I am sorry, but I am in very strong opposition  against this idea. Nobody
> did reply to my questions, that can change my opinion.

From your reply it's not quite clear, are you opposed to have a mandatory
VARIABLE syntax, or having variables in the FROM clause? My main proposal was
about the former, but the points that are following seems to talk about the
latter. I think it's fine to reject the idea about the FROM clause, as long as
you got some reasonable arguments.

> > Then dropping the column b, but everything still works beause the
> > variable b got silently picked up. But if it would be required to say
> > VARIABLE(b), then all fine.
>
> but same risk you have any time in plpgsql - all time. I don't remember any
> bug report related to this issue.

Which exactly scenario about plpgsql do you have in mind? Just have tried to
declare a variable inside a plpgsql function with the same name as a table
column, and got an error about an ambiguous reference.

> Theoretically, variables can have the same names as tables. The table
> overshadows the variable, so it can work. But when somebody drops the
> variable, then the query still can work. So requirement of usage variable
> in FROM clause protects us just against drop column, but not against
> dropping table. In Postgres the dropping table is possibly risky due
> search_path (that introduces shadowing concept) without introduction
> variables. There is a possibility of this issue, but how common is this
> issue?

This sounds to me like an argument against allowing name clashing between
variables and tables. It makes even more sense, since session variables are in
many ways similar to tables.

> I think this issue can be partially similar to creating two equally named
> tables in different schemas (both schemas are in search path). When you
> drop one table, the query will work, but the result is different. It is the
> same issue. The SQL has no concept of shadowing and on the base line it is
> not necessary.

The point is that most of users are aware about schemas and search path
dangers. But to me such a precedent is not an excuse to introduce a new feature
with similar traps, which folks would have to learn by making mistakes. Judging
from the feedback to this patch over time, I've got an impression that lots of
people are also not fans of that.

> > Then dropping the column b, but everything still works beause the
> > variable b got silently picked up. But if it would be required to say
> > VARIABLE(b), then all fine.
> >
>
> In this scenario you will get  a warning related to variable shadowing
> (before you drop a column).
>
> [...]
>
> What do you think about the following design?  I can implement a warning
> "variable_usage_guard" when the variable is accessed without using
> VARIABLE() syntax. We can discuss later if this warning can be enabled by
> default or not. There I am open to any variant.

I don't follow what are you winning by that? In the context of problem above
(i.e. dropping a column), such a warning is functionally equivalend to a
warning about shadowing.

The problem is that it doesn't sound very appealing to have a feature, which
requires some extra efforts to be used in a right way (e.g. put everything into
a special vars schema, or keep an eye on logs). Most certainly there are such
bits in PostgreSQL today, with all the best practices, crowd wisdom, etc. But
the bar for new features in this sense is much higher, you can see it from the
feedback to this patch. Thus I believe it makes sense, from purely tactical
reasons, to not try to convince half of the community to lower the bar, but
instead try to modify the feature to make it more acceptable, even if some
parts you might not like.

Btw, could you repeat, what was exactly your argument against mandatory
VARIABLE() syntax? It's somehow scattered across many replies, would be great
to summarize it in a couple of phrases.

> Shadowing by self is not an issue, probably, but it is a signal of code
> quality problems.

Agree, but I'm afraid code quality of an average application using PostgreSQL
is quite low, so here we are.

As a side note, I've recently caught myself thinking "it would be cool to have
session variables here". The use case was preparing a policy for RLS, based on
some session-level data set by an application. This session-level data is of a
composite data type, so simple current_setting is cumbersome to use, and a
temporary table will be dropped at the end, taking the policy with it due to
the recorded dependency between them. Thus a session variable of some composite
type sounds like a good fit.

Dmitry Dolgov:
> This sounds to me like an argument against allowing name clashing between
> variables and tables. It makes even more sense, since session variables are in
> many ways similar to tables.

+1

My mental model of a session variable is similar to a single-row, 
optionally global temporary, table.

Is there any substantial difference that I am not aware of?

Best,

Wolfgang

Pavel Stehule:
> (global (temp)) table can hold 0, 1 or more rows (and rows are always 
> composite of 0..n fields). The variable holds a value of some type. 
> Proposed session variables are like plpgsql variables (only with 
> different scope). In Postgres there is a difference between a scalar 
> variable and composite variable with one field. 

I can store composite values in table columns, too. A table column can 
either be scalar or composite in that sense.

So, maybe rephrase: Single-row, single-column (global (temp)) table = 
variable. One "cell" of that table.

For me, the major difference between a variable and a table is, that the 
table has 0...n rows and 0...m columns, while the variable has *exactly* 
one in both cases, not 0 either.

I must put tables into FROM, why not those nice mini-tables called 
variables as well? Because they are potentially scalar, you say!

But: I can already put functions returning scalar values into FROM:

   SELECT * FROM format('hello');

The function returns a plain string only.

I don't know. This just "fits" for me.

Or to put it differently: I don't really care whether I have to write 
"(SELECT variable FROM variable)" instead of just "variable". I don't 
want session variables for the syntax, I want session variables, because 
they are **so much better** than custom GUCs.

Best,

Wolfgang

> On Tue, Nov 19, 2024 at 08:14:01PM +0100, Pavel Stehule wrote:
> Hi
>
> I wrote POC of VARIABLE(varname) syntax support

Thanks, the results look good. I'm still opposed the idea of having a
warning, and think it has to be an error -- but it's my subjective
opinion. Lets see if there are more votes on that topic.

On Thu, Dec 5, 2024 at 2:52 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
> only rebase

hi.
disclaimer, i *only* applied
v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch.

create variable v2 as text;
alter variable v2 rename to v2;
ERROR:  session variable "v2" already exists in schema "public"

the above is  coverage tests for report_namespace_conflict:
        case VariableRelationId:
            Assert(OidIsValid(nspOid));
            msgfmt = gettext_noop("session variable \"%s\" already
exists in schema \"%s\"");
            break;

create type t1 as (a int, b int);
CREATE VARIABLE var1 AS t1;
alter type t1 drop attribute a;
should "alter type t1 drop attribute a;" not allowed?


GetCommandLogLevel also needs to deal with  case T_CreateSessionVarStmt?


there are no regress tests for the change we made in
find_composite_type_dependencies?
It looks like it will be reachable for sure.


CreateVariable, print out error position:
    if (get_typtype(typid) == TYPTYPE_PSEUDO)
        ereport(ERROR,
                (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                 errmsg("session variable cannot be pseudo-type %s",
                        format_type_be(typid)),
                 parser_errposition(pstate, stmt->typeName->location)));

Alvaro Herrera told me actually, you don't need the extra parentheses
around errcode.
so you can:
    if (get_typtype(typid) == TYPTYPE_PSEUDO)
        ereport(ERROR,
                errcode(ERRCODE_WRONG_OBJECT_TYPE),
                errmsg("session variable cannot be pseudo-type %s",
                        format_type_be(typid)),
                parser_errposition(pstate, stmt->typeName->location))


pg_variable_is_visible seems to have done twice the system cache call.
maybe follow through with the pg_table_is_visible, pg_type_is_visible
code pattern.


IN src/bin/psql/describe.c
+ appendPQExpBufferStr(&buf, "\nWHERE true\n");
this is not needed?
------------------------------------------------
some of the `foreach` can change to foreach_oid, foreach_node
see: https://git.postgresql.org/cgit/postgresql.git/commit/?id=14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
------------------------------------------------
doc/src/sgml/ref/create_variable.sgml
<programlisting>
CREATE VARIABLE var1 AS date;
LET var1 = current_date;
SELECT var1;
</programlisting>

v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
alone cannot do `LET var1 = current_date;`, `SELECT var1;`
maybe the following patch can do it. but that makes
we cannot commit
v20241205-0001-Enhancing-catalog-for-support-session-variables-and-.patch
alone.
------------------------------------------------
since CREATE VARIABLE is an extension to standard, so in create_schema.sgml
Compatibility section,
do we need to mention CREATE SCHEMA CREATE VARIABLE is an extension
from standard
?
-----------------------------------------------

/*
 * Drop variable by OID, and register the needed session variable
 * cleanup.
 */
void
DropVariableById(Oid varid)
i am not sure of the meaning of "register the needed session variable cleanup".

On Mon, Dec 9, 2024 at 2:33 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
again. only applied
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch.

/* we want SessionVariableCreatePostprocess to see the catalog changes. */
0001 doesn't have SessionVariableCreatePostprocess,
so this comment is wrong in the context of 0001.

typo:
As above, but if the variable isn't found and is_mussing is not NULL
is_mussing should be is_missing.

----------------------------------------------
issue with  grant.sgml and revoke.sgml.

* there are no regress tests for WITH GRANT OPTION but it seems to work;
there are no REVOKE CASCADE tests. the following tests show
REVOKE CASCADE works.

create variable v1 as int;
GRANT select on VARIABLE v1 to alice with grant option;
set session authorization alice;
GRANT select on VARIABLE v1 to bob with grant option;
reset session authorization;

select varacl from pg_variable where varname  = 'v1';
--output
{jian=rw/jian,alice=r*/jian,bob=r*/alice}
revoke all privileges on variable v1 from alice cascade;
select varacl from pg_variable where varname  = 'v1';
--output
 {jian=rw/jian}

revoke GRANT OPTION FOR all privileges on variable v1 from alice cascade;
also works.

* in revoke.sgml and grant.sgml.
if you look closely, " | ALL VARIABLES IN SCHEMA schema_name [, ...] }" is wrong
because there is no left-curly-bracket, but there is a right-curly-bracket.

* in revoke.sgml.
<phrase>where <replaceable
class="parameter">role_specification</replaceable> can be:</phrase>
    [ GROUP ] <replaceable class="parameter">role_name</replaceable>
  | PUBLIC
  | CURRENT_ROLE
  | CURRENT_USER
  | SESSION_USER
should be at the end of the synopsis section?
otherwise it looks weird, maybe we can put the REVOKE VARIABLE code upper.
grant.sgml changes position looks fine to me.


*  <para>
   The <command>GRANT</command> command has two basic variants: one
   that grants privileges on a database object (table, column, view,
   foreign table, sequence, database, foreign-data wrapper, foreign server,
   function, procedure, procedural language, large object, configuration
   parameter, schema, tablespace, or type), and one that grants
   membership in a role.  These variants are similar in many ways, but
   they are different enough to be described separately.
  </para>
this <para> in grant.sgml needs to also mention "variable"?

*   <para>
    Privileges on databases, tablespaces, schemas, languages, and
    configuration parameters are
    <productname>PostgreSQL</productname> extensions.
   </para>
this <para> in grant.sgml needs to also mention "variable"?

----------------------------------------------
*
+   <para>
+    Inside a query or an expression, a session variable can be
+    <quote>shadowed</quote> by a column with the same name.  Similarly, the
+    name of a function or procedure argument or a PL/pgSQL variable (see
PL/pgSQL should decorated as <application>PL/pgSQL</application>

* we already support \dV and \dV+ in
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch
so we should update doc/src/sgml/ref/psql-ref.sgml also.
I briefly searched \dV in
v20241208-0002-Storage-for-session-variables-and-SQL-interface.patch,
no entry.


* in doc/src/sgml/ddl.sgml
<table id="privilege-abbrevs-table"> and <table
id="privileges-summary-table"> need to change for variable?
<varlistentry id="ddl-priv-select">, <varlistentry
id="ddl-priv-update"> need to change for variable?

it's kind of tricky. if we only apply
v20241208-0001-Enhancing-catalog-for-support-session-variables-and-.patch
we can not  SELECT or  UPDATE variable.
so how are we going to mention these privileges for variable?


 * we can add some tests for EVENT TRIGGER test,
since event trigger support CREATE|DROP variable. atually, I think
there is a bug.

create variable v1 as int;
CREATE OR REPLACE FUNCTION event_trigger_report_dropped()
RETURNS event_trigger
LANGUAGE plpgsql
AS $$
DECLARE r record;
BEGIN
FOR r IN SELECT * from pg_event_trigger_dropped_objects()
LOOP
IF NOT r.normal AND NOT r.original THEN
    CONTINUE;
END IF;
RAISE NOTICE 'NORMAL: orig=% normal=% istemp=% type=% identity=% name=% args=%',
    r.original, r.normal, r.is_temporary, r.object_type,
    r.object_identity, r.address_names, r.address_args;
END LOOP;
END; $$;

CREATE EVENT TRIGGER regress_event_trigger_report_dropped ON sql_drop
WHEN TAG IN ('DROP VARIABLE')
EXECUTE PROCEDURE event_trigger_report_dropped();

--output:
src9=# drop variable v1;
NOTICE:  test_event_trigger: ddl_command_start DROP VARIABLE
NOTICE:  NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,$} args={}
DROP VARIABLE

should i expect
NOTICE:  NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,$} args={}
to be
NOTICE:  NORMAL: orig=t normal=f istemp=f type=session variable
identity=public.v1 name={public,v1} args={}
?

hi.

GRANT|REVOKE  ALL VARIABLES IN SCHEMA schema_name [, ...] }
seems to work.
might be better to add tests.

also src/bin/psql/tab-complete.in.c
COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_grantables,
we can also add "ALL VARIABLES IN SCHEMA "

also need change this <para> in grant.sgml:
  <para>
   There is also an option to grant privileges on all objects of the same
   type within one or more schemas.  This functionality is currently supported
   only for tables, sequences, functions, and procedures.  <literal>ALL
   TABLES</literal> also affects views and foreign tables, just like the
   specific-object <command>GRANT</command> command.  <literal>ALL
   FUNCTIONS</literal> also affects aggregate and window functions, but not
   procedures, again just like the specific-object <command>GRANT</command>
   command.  Use <literal>ALL ROUTINES</literal> to include procedures.
  </para>

revoke.sgml, we should use <replaceable
class="parameter">role_specification</replaceable>?
so it will become like:

REVOKE [ GRANT OPTION FOR ]
    { { SELECT | UPDATE } [, ...] | ALL [ PRIVILEGES ] }
    ON { VARIABLE variable_name [, ...]
       | ALL VARIABLES IN SCHEMA schema_name [, ...] }
    FROM role_specification [, ...]

maybe also add
[ GRANTED BY role_specification ]
but I didn't test "REVOKE [ GRANTED BY role_specification ]".

Speaking of acl tests,
similar to has_table_privilege I am afraid we need to have a function
like has_variable_privilege for acl tests.
has_table_privilege has 6 function signatures. so there will be more code.
------------------------------------------------------
doc/src/sgml/ref/create_variable.sgml
<synopsis> section:
CREATE VARIABLE [ IF NOT EXISTS ] name [ AS ] data_type ] [ COLLATE collation ]

redundant right square bracket after "data_type".

hi.

/*
 * has_session_variable_privilege variants
 *        These are all named "has_session_variable_privilege" at the SQL level.
 *        They take various combinations of variable name, variable OID,
 *        user name, user OID, or implicit user = current_user.
 *
 *        The result is a boolean value: true if user has the indicated
 *        privilege, false if not.  The variants that take a relation OID
 *        return NULL if the OID doesn't exist.
 */
/*
 * has_session_variable_privilege_name_name
 *        Check user privileges on a session variable given
 *        name username, text sessin variable name, and text priv name.
 */
"The variants that take a relation OID return NULL if the OID doesn't exist."
should it be
"The variants that take an OID type return NULL if the OID doesn't exist."
?

typo, "sessin" should be "session".
----------------<<<>>>>-------------------
  <sect1 id="ddl-session-variables">
   <title>Session Variables</title>
only mentioned that "Session variables themselves are persistent, but their
values are neither persistent nor shared (like the content of temporary tables).
"
I feel like this sentence is not that explicit. we actually want to say
"Once a session exits, the variable value is reset to NULL, one
session cannot see another session variable value."

+    <para>
+     A persistent database object that holds a value in session memory.  This
+     value is private to each session and is released when the session ends.
+     Read or write access to session variables is controlled by privileges,
+     similar to other database objects.
+    </para>
i do like this description in glossary.sgml.
maybe we can copy it and put it to ddl.sgml "<sect1 id="ddl-session-variables">
----------------<<<>>>>-------------------
REVOKE [ GRANT OPTION FOR ]
    { { SELECT | UPDATE } [, ...] | ALL [ PRIVILEGES ] }
    ON { VARIABLE <replaceable>variable_name</replaceable> [, ...]
       | ALL VARIABLES IN SCHEMA <replaceable
class="parameter">schema_name</replaceable> [, ...] }
    FROM { [ GROUP ] <replaceable
class="parameter">role_specification</replaceable> | PUBLIC } [, ...]
    [ GRANTED BY <replaceable
class="parameter">role_specification</replaceable> ]
    [ CASCADE | RESTRICT ]
revoke, seems still not right.
since with this, we can say:
REVOKE ALL PRIVILEGES ON VARIABLE v1 FROM group group alice CASCADE;

i think the correct one should be:
REVOKE [ GRANT OPTION FOR ]
    { { SELECT | UPDATE } [, ...] | ALL [ PRIVILEGES ] }
    ON { VARIABLE <replaceable>variable_name</replaceable> [, ...]
       | ALL VARIABLES IN SCHEMA <replaceable
class="parameter">schema_name</replaceable> [, ...] }
    FROM <replaceable class="parameter">role_specification</replaceable> [, ...]
    [ GRANTED BY <replaceable
class="parameter">role_specification</replaceable> ]
    [ CASCADE | RESTRICT ]

----------------<<<>>>>-------------------
<programlisting>
CREATE VARIABLE public.current_user_id AS integer;
GRANT READ ON VARIABLE public.current_user_id TO PUBLIC;
LET current_user_id = (SELECT id FROM users WHERE usename = session_user);
SELECT current_user_id;
</programlisting>
"GRANT READ" should be "GRANT SELECT".
----------------<<<>>>>-------------------
doc/src/sgml/ref/alter_default_privileges.sgml
GRANT { SELECT | UPDATE | ALL [ PRIVILEGES ] }
    ON VARIABLES
    TO { [ GROUP ] <replaceable
class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH
GRANT OPTION ]
the above part is wrong?
should be:
GRANT { { SELECT | UPDATE } [,...]
    | ALL [ PRIVILEGES ] }
    ON VARIABLES
    TO { [ GROUP ] <replaceable
class="parameter">role_name</replaceable> | PUBLIC } [, ...] [ WITH
GRANT OPTION ]

since we can:
ALTER DEFAULT PRIVILEGES
FOR ROLE alice
IN SCHEMA svartest
GRANT SELECT, UPDATE ON VARIABLES TO bob;
----------------<<<>>>>-----------------------------
CREATE VARIABLE IF NOT EXISTS v2 AS comp;
grant update on variable v2 to alice;
set role alice;
LET v2.a  = 12; --acl permission error
LET v2.b = 12; --acl permission error
LET v2 = (11,12); --ok.

not sure this is the desired behavior, for composite type variables, you are
allowed to change all the values, but you are not allowed to update the field
value of the composite.  The following are normal table test update cases.

create type comp as (a int, b  int);
create table t2(a comp);
insert into t2 select '(11,12)';
grant update (a ) on t2 to alice;
set role alice;
update t2 set a.a = 13; --ok
update t2 set a.b = 13; --ok
update t2 set a = '(11,13)'; --ok
----------------<<<>>>>-----------------------------
domain seems to have an issue.

CREATE domain d1 AS int;
CREATE VARIABLE var1 AS d1;
let var1 = 3;
--this should fail?.
alter domain d1 add check (value <> 3);
select var1;
ERROR:  value for domain d1 violates check constraint "d1_check"
----------------<<<>>>>-----------------------------
doc/src/sgml/ref/alter_variable.sgml
<title>Parameters</title> section, the order should
be: name, new_owner, new_name, new_schema?

I am beginning to look around 0002.

hi.
review is based on
v20241219-0002-Storage-for-session-variables-and-SQL-interface.patch
and
v20241219-0001-Enhancing-catalog-for-support-session-variables-and-.patch.

in doc/src/sgml/catalogs.sgml

<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>defaclobjtype</structfield> <type>char</type>
</para>
<para>
Type of object this entry is for:
<literal>r</literal> = relation (table, view),
<literal>S</literal> = sequence,
<literal>f</literal> = function,
<literal>T</literal> = type,
<literal>n</literal> = schema
</para></entry>
</row>
this need updated for session variable?

psql meta command \ddp
src/bin/psql/describe.c listDefaultACLs
also need to change.
----------------<<>>-------------------------
+-- show variable
+SELECT public.svar;
+SELECT public.svar.c;
+SELECT (public.svar).*;
+
+-- the variable is shadowed, raise error
+SELECT public.svar.c FROM public.svar;
+
+-- can be fixed by alias
+SELECT public.svar.c FROM public.svar x

The above tests are duplicated in session_variables.sql.
----------------<<>>-------------------------
--- a/src/include/nodes/plannodes.h
+++ b/src/include/nodes/plannodes.h
@@ -49,7 +49,7 @@ typedef struct PlannedStmt

     NodeTag        type;

-    CmdType        commandType;    /*
select|insert|update|delete|merge|utility */
+    CmdType        commandType;    /*
select|let|insert|update|delete|merge|utility */

since we don't have CMD_LET CmdType.
so this comment change is not necessary?
----------------<<>>-------------------------
the following are simple tests that triggers makeParamSessionVariable error
messages. we don't have related tests, we can add it on session_variables.sql.

create type t1 as (a int, b int);
CREATE VARIABLE v1 text;
CREATE VARIABLE v2 as t1;
select v1.a;
select v2.c;

also
select v2.c;
ERROR:  could not identify column "c" in variable
LINE 1: select v2.c;
               ^
the error message is no good. i think we want:
ERROR:  could not identify column "c" in  variable "public.v1"
----------------<<>>-------------------------
+    /*
+     * Check for duplicates. Note that this does not really prevent
+     * duplicates, it's here just to provide nicer error message in common
+     * case. The real protection is the unique key on the catalog.
+     */
+    if (SearchSysCacheExists2(VARIABLENAMENSP,
+                              PointerGetDatum(varName),
+                              ObjectIdGetDatum(varNamespace)))
+    {
+    }
I am confused by these comments. i think the SearchSysCacheExists2
do actually prevent duplicates.
I noticed that publication_add_relation also has similar comments.
----------------<<>>-------------------------
typedef struct LetStmt
{
    NodeTag        type;
    List       *target;            /* target variable */
    Node       *query;            /* source expression */
    int            location;
} LetStmt;
here, location should be a type of ParseLoc.
----------------<<>>-------------------------
in 0001, 0002, function SetSessionVariableWithSecurityCheck
never being used.
----------------<<>>-------------------------
+/*
+ * transformLetStmt -
+ *      transform an Let Statement
+ */
+static Query *
+transformLetStmt(ParseState *pstate, LetStmt *stmt)
...
+    /*
+     * Save target session variable ID.  This value is used multiple times: by
+     * the query rewriter (for getting related defexpr), by planner (for
+     * acquiring an AccessShareLock on target variable), and by the executor
+     * (we need to know the target variable ID).
+     */
+    query->resultVariable = varid;

"defexpr", do you mean "default expression"?
Generally letsmt is like: "let variable = (SelectStmt)"
is there nothing related to the DEFAULT expression?
"(we need to know the target variable ID)." in ExecuteLetStmt that is true.
but I commented out the following lines, the regress test still
succeeded.

extract_query_dependencies_walker
/* record dependency on the target variable of a LET command */
// if (OidIsValid(query->resultVariable))
//     record_plan_variable_dependency(context, query->resultVariable);

ScanQueryForLocks
// /* process session variables */
// if (OidIsValid(parsetree->resultVariable))
// {
//     if (acquire)
//         LockDatabaseObject(VariableRelationId, parsetree->resultVariable,
//                            0, AccessShareLock);
//     else
//         UnlockDatabaseObject(VariableRelationId, parsetree->resultVariable,
//                              0, AccessShareLock);
// }
----------------<<>>-------------------------
in transformLetStmt, we already did ACL privilege check,
we don't need do it again at ExecuteLetStmt?

hi.

src9=# select 'XLogRecPtr'::regtype;
ERROR:  type "xlogrecptr" does not exist
LINE 1: select 'XLogRecPtr'::regtype;
               ^
so
+ <structfield>varcreate_lsn</structfield> <type>XLogRecPtr</type>
should be
<structfield>varcreate_lsn</structfield> <type>pg_lsn</type>
?

also
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>varcreate_lsn</structfield> <type>XLogRecPtr</type>
+      </para>
+      <para>
+       LSN of the transaction where the variable was created.
+       <structfield>varcreate_lsn</structfield> and
+       <structfield>oid</structfield> together form the all-time unique
+       identifier (<structfield>oid</structfield> alone is not enough, since
+       object identifiers can get reused).
+      </para></entry>
+     </row>
+
we have "pg_variable_oid_index" PRIMARY KEY, btree (oid)
for table pg_variable.
so I am confused by saying the column "oid" itself is not enough to
prove unique.

in let.sgml
<term><literal>session_variable</literal></term>
should be
<term><replaceable class="parameter">session_variable</replaceable></term>

<term><literal>sql_expression</literal></term>
should be
<term><replaceable class="parameter">sql_expression</replaceable></term>

hi.

+ if (stmt->collClause)
+ collation = LookupCollation(pstate,
+ stmt->collClause->collname,
+ stmt->collClause->location);
+ else
+ collation = typcollation;;

two semi-colon. should be only one.
------------------<<>>>---------------
+ /* locks the variable with an AccessShareLock */
+ varid = IdentifyVariable(names, &attrname, ¬_unique, false);
+ if (not_unique)
+ ereport(ERROR,
+ (errcode(ERRCODE_AMBIGUOUS_PARAMETER),
+ errmsg("target \"%s\" of LET command is ambiguous",
+ NameListToString(names)),
+ parser_errposition(pstate, stmt->location)));

the following are tests for the above "LET command is ambiguous" error message.

create schema test;
CREATE TYPE test AS (test int);
CREATE variable test.test as test;
set search_path to test;
let test.test = 1;
------------------<<>>>---------------
+ else
+ {
+ /* the last field of list can be star too */
+ Assert(IsA(field2, A_Star));
+
+ /*
+ * In this case, the field1 should be variable name. But
+ * direct unboxing of composite session variables is not
+ * supported now, and then we don't need to try lookup
+ * related variable.
+ *
+ * Unboxing is supported by syntax (var).*
+ */
+ return InvalidOid;
+ }
I don't fully understand the above comments,
add
`elog(INFO, "%s:%d called", __FILE__, __LINE__); ` within the ELSE branch.
Then I found out the ELSE branch doesn't have coverage tests.

------------------<<>>>---------------
+ /*
+ * a.b.c can mean catalog.schema.variable or
+ * schema.variable.field.
....
+ /*
+ * a.b can mean "schema"."variable" or "variable"."field".
+ * Check both variants, and returns InvalidOid with
+ * not_unique flag, when both interpretations are
+ * possible.
+ */
here, we use the word "field", but the function IdentifyVariable above
comment, we use
word "attribute", for consistency's sake, we should use "attribute"
instead of "field"

+/* -----
+ * IdentifyVariable - try to find a variable from a list of identifiers
+ *
+ * Returns the OID of the variable found, or InvalidOid.
+ *
+ * "names" is a list of up to four identifiers; possible meanings are:
+ * - variable  (searched on the search_path)
+ * - schema.variable
+ * - variable.attribute  (searched on the search_path)
+ * - schema.variable.attribute
+ * - database.schema.variable
+ * - database.schema.variable.attribute

------------------<<>>>---------------

On Sun, Dec 29, 2024 at 5:50 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
> Hi
>
>
>> ------------------<<>>>---------------
>> + else
>> + {
>> + /* the last field of list can be star too */
>> + Assert(IsA(field2, A_Star));
>> +
>> + /*
>> + * In this case, the field1 should be variable name. But
>> + * direct unboxing of composite session variables is not
>> + * supported now, and then we don't need to try lookup
>> + * related variable.
>> + *
>> + * Unboxing is supported by syntax (var).*
>> + */
>> + return InvalidOid;
>> + }
>> I don't fully understand the above comments,
>
>
> The parser allows only two syntaxes - identifier.identifier or identifier.star. Second
> syntax is not supported by session variables, and then I didn't try to search for the variable.
> Some users can be confused by similar syntaxes identifier.* and (identifier).* Only
> second syntax is composite unboxing, and only second syntax is supported for
> session variables.
>
> Maybe the note about unboxing is messy there?
>
>> add
>> `elog(INFO, "%s:%d called", __FILE__, __LINE__); ` within the ELSE branch.
>> Then I found out the ELSE branch doesn't have coverage tests.
>
>
> I don't understand this comment? I don't use elog(INFO anywhere
>
>

sorry for confusion, i mean,
i added " elog(INFO", the regress test is still successful,
therefore it means the above ELSE branch code doesn't have coverage tests.

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 9c2957eb546..624858db301 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -361,6 +361,9 @@ typedef struct Const
  * of the `paramid' field contain the SubLink's subLinkId, and
  * the low-order 16 bits contain the column number.  (This type
  * of Param is also converted to PARAM_EXEC during planning.)
+ *
+ * PARAM_VARIABLE:  The parameter is a reference to a session variable
+ * (paramid holds the variable's OID).
  */
 typedef enum ParamKind
 {
@@ -368,6 +371,7 @@ typedef enum ParamKind
  PARAM_EXEC,
  PARAM_SUBLINK,
  PARAM_MULTIEXPR,
+ PARAM_VARIABLE,
 } ParamKind;

 typedef struct Param
@@ -380,6 +384,10 @@ typedef struct Param
  int32 paramtypmod pg_node_attr(query_jumble_ignore);
  /* OID of collation, or InvalidOid if none */
  Oid paramcollid pg_node_attr(query_jumble_ignore);
+ /* OID of session variable if it is used */
+ Oid paramvarid;
+ /* true when param is used like base node of assignment indirection */
+ bool parambasenode;
  /* token location, or -1 if unknown */
  ParseLoc location;
 } Param;

comment should be "(paramvarid holds the variable's OID)"
also
+ /* true when param is used like base node of assignment indirection */
is kind of hard to understand.
param->parambasenode = true;
only happens in transformLetStmt so we can change it to
+ /* true when param is used in part of the LET statement.*/


--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -51,6 +51,10 @@ typedef struct QueryDesc
  /* This field is set by ExecutePlan */
  bool already_executed; /* true if previously executed */

+ /* reference to session variables buffer */
+ int num_session_variables;
+ SessionVariableValue *session_variables;
+
  /* This is always set NULL by the core system, but plugins can change it */
  struct Instrumentation *totaltime; /* total time spent in ExecutorRun */
 } QueryDesc;
QueryDesc->num_session_variables and
QueryDesc->session_variables are never used in 0001 and 0002.
it may be used in later patches,
but at least 0001 and 0002, we don't need to change
struct QueryDesc?


contrib/postgres_fdw/deparse.c
There are some cases of "case T_Param:"
do we need to do something on it?
also on src/backend/nodes/queryjumblefuncs.c, one
appearance of "case T_Param:". (but it seems no need change here)



CREATE VARIABLE v1 AS int;
CREATE VARIABLE v2 AS int;
SELECT pg_stat_statements_reset() IS NOT NULL AS t;
select count(*) from tenk1 where unique1 = v1;
select count(*) from tenk1 where unique1 = v2;

should the last two queries be normalized as one query in pg_stat_statements?
if so, then maybe in typedef struct Param
we need change to:
Oid            paramvarid pg_node_attr(query_jumble_ignore);
but then
let v1 = v1+1;
let v1 = v2+1;
will be normalized as one query.

+ /*
+ * The arguments of EXECUTE are evaluated by a direct expression
+ * executor call.  This mode doesn't support session variables yet.
+ * It will be enabled later.
+ */
+ if (pstate->p_hasSessionVariables)
+ elog(ERROR, "session variable cannot be used as an argument");

it should be:
    /*
     * The arguments of CALL statement are evaluated by a direct expression
     * executor call.  This path is unsupported yet, so block it.
     */
    if (pstate->p_hasSessionVariables)
        ereport(ERROR,
                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                errmsg("session variable cannot be used as an argument"));


similarly, EvaluateParams we can change it to
    /*
     * The arguments of EXECUTE are evaluated by a direct expression
     * executor call.  This mode doesn't support session variables yet.
     * It will be enabled later.
     */
    if (pstate->p_hasSessionVariables)
        ereport(ERROR,
                errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                errmsg("session variable cannot be used as an argument"));

in src/backend/executor/execExpr.c
we don't need
+#include "catalog/pg_variable.h"
?

hi.

some minor issues.
'transformMergeStmt also needs
"qry->hasSessionVariables = pstate->p_hasSessionVariables;"
?


<function>acldefault</function> in doc/src/sgml/func.sgml
Need an update for SESSION VARIABLE?

Table 9.70. Access Privilege Inquiry Functions
sorting order: has_session_variable_privilege should after has_server_privilege?


Similar to src/test/regress/sql/event_trigger.sql,
we can use the following query to test four functions in
Table 9.79. Object Information and Addressing Functions
(9.27.5. Object Information and Addressing Functions)
for session variables.

SELECT
  e.varname,
  pg_describe_object('pg_variable'::regclass, e.oid, 0) as descr,
  b.type, b.object_names, b.object_args,
  pg_identify_object(a.classid, a.objid, a.objsubid) as ident
FROM pg_variable as e,
LATERAL pg_identify_object_as_address('pg_variable'::regclass, e.oid, 0) as b,
LATERAL pg_get_object_address(b.type, b.object_names, b.object_args) as a;


in function transformColumnRef
if (expr_kind_allows_session_variables(pstate->p_expr_kind))
{
}
can change to
if (!node &&
    !(relname && crerr == CRERR_NO_COLUMN) &&
    expr_kind_allows_session_variables(pstate->p_expr_kind))
{
}
can reduce the function call of expr_kind_allows_session_variables,
also not lose readability.


typedef struct SVariableData says:
    /*
     * Stored value and type description can be outdated when we receive a
     * sinval message.  We then have to check if the stored data are still
     * trustworthy.
     */
    bool        is_valid;

CREATE VARIABLE var3 AS int;
LET var3 = 10;
GRANT SELECT ON VARIABLE var3 TO regress_noowner;
I don't understand why the last statement GRANT SELECT needs to call
pg_variable_cache_callback?
and it will invalidate the original var3.


+/*
+ * Returns a copy of the value of the session variable (in the current memory
+ * context).  The caller is responsible for permission checks.
+ */
+Datum
+GetSessionVariable(Oid varid, bool *isNull, Oid *typid)
+{
+ SVariable svar;
+
+ svar = get_session_variable(varid);
+
+ /*
+ * Although "svar" is freshly validated in this point, svar->is_valid can
+ * be false, if an invalidation message ws processed during the domain check.
+ * But the variable and all its dependencies are locked now, so we don't need
+ * to repeat the validation.
+ */
+ Assert(svar);
+
+ *typid = svar->typid;
+
+ return copy_session_variable_value(svar, isNull);
+}
typo, "ws processed" should be "was processed".
also the Assert is not helpful? since svar is NULL,
get_session_variable would have segfault.
also SVariable svar; is not initialized to NULL, so generally it will
not be NULL by default.
also the comments seem to imply that before
copy_session_variable_value, svar->is_valid can be false.
if svar->is_valid is false, then why do copy_session_variable_value.

hi.

you forgot change
<function>acldefault</function>
should add
'V' for <literal>SESSION VARIABLE</literal>


in doc/src/sgml/ddl.sgml
<sect1 id="ddl-session-variables">
maybe some examples (<programlisting>) of session variables being
shadowed would be great.

because doc/src/sgml/ref/create_variable.sgml said
  <note>
   <para>
    Session variables can be <quote>shadowed</quote> by other identifiers.
    For details, see <xref linkend="ddl-session-variables"/>.
   </para>
  </note>
If I click the link, then in ddl.sgml, there is also a bunch of text
saying that variable will be shadowed
in some situations, but there are no simple examples to demonstrate that.


"Create an date session variable <literal>var1</literal>:"
maybe it should be rephrased as
"Create a session variable <literal>var1</literal> as date data type?"
(i am not native english speaker)

-----------------------------------------------------------------------
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -379,7 +379,8 @@ typedef struct Param
        Expr            xpr;
        ParamKind       paramkind;              /* kind of parameter.
See above */
        int                     paramid;                /* numeric ID
for parameter */
-       Oid                     paramtype;              /* pg_type OID
of parameter's datatype */
+       /* pg_type OID of parameter's datatype */
+       Oid                     paramtype
pg_node_attr(query_jumble_ignore);

I think we need the above to make
select v2;
select v1;
normalized as one query.
-----------------------------------------------------------------------
when we create a new table, we do something like this:
DefineRelation
heap_create_with_catalog
GetNewRelFileNumber
GetNewOidWithIndex

relation Oid uniqueness and variable uniqueness is the same thing.
If variable oid uniqueness problem ever reached,
at that moment, we should care more about relation oid uniqueness problem?

and in GetNewRelFileNumber, we have comments:
""
 * As with GetNewOidWithIndex(), there is some theoretical risk of a race
 * condition, but it doesn't seem worth worrying about.
"""
also comments in GetNewOidWithIndex
"""
 * Note that we are effectively assuming that the table has a relatively small
 * number of entries (much less than 2^32) and there aren't very long runs of
 * consecutive existing OIDs.  This is a mostly reasonable assumption for
 * system catalogs.
"""
that means pg_catalog.pg_variable.varcreate_lsn is not really necessary?
-----------------------------------------------------------------------
I think the latest patch for LET self assign ACL SELECT is not correct,
previously I also tried the same idea.

test demo.
CREATE TYPE vartest_t1 AS (a int, b int);
CREATE VARIABLE var1 AS vartest_t1;
CREATE ROLE regress_var_test_role;
GRANT UPDATE ON VARIABLE var1 TO regress_var_test_role;
GRANT select ON table tenk1 TO regress_var_test_role;
SET ROLE TO regress_var_test_role;

--both should fail
LET var1.a = var1.a + 10;
LET var1.a = (select * from (select count(*) from tenk1 where unique1
= var1.a + 10));
--------------------------------------------------------

On Fri, Jan 17, 2025 at 08:18:20AM +0100, Pavel Stehule wrote:
> Hi
> 
> fix oid collision

What is the purpose of continually posting this patch to the email
lists?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Fri, Jan 17, 2025 at 02:48:29PM +0100, Pavel Stehule wrote:
> Hi
> 
> pá 17. 1. 2025 v 14:41 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> 
>     On Fri, Jan 17, 2025 at 08:18:20AM +0100, Pavel Stehule wrote:
>     > Hi
>     >
>     > fix oid collision
> 
>     What is the purpose of continually posting this patch to the email
>     lists?
> 
> 
> The people still do a review, so I am fixing this patch. I am fixing it
> immediately, when I detect an issue.
> 
> I can reduce the frequency once per week.

Is this really something we are considering applying, since it has been
around for years?  I am unclear on that and we had better know if we are
going to continue reviewing this.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On 2025-Jan-17, Bruce Momjian wrote:

> Is this really something we are considering applying, since it has been
> around for years?  I am unclear on that and we had better know if we are
> going to continue reviewing this.

The fact that the patch has been around for years doesn't automatically
mean it's a bad idea.

I have proposed that we discuss this patch at fosdem developer's meeting
next month, precisely to seek consensus on whether this patch is
something we want or not.  My view is that this is a feature that has
been requested by users for years, so IMO we want this or something
similar.

I wonder if the reason that committers stay away from it is that
reviewing it fully (and thus taking responsibility for it) seems such a
daunting task.  I might be wrong, but I think this may be the largest
patch since FTS.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
                               http://postgr.es/m/482D1632.8010507@sigaev.ru

On Fri, Jan 17, 2025 at 04:32:07PM +0100, Pavel Stehule wrote:
> This discussion was around 2017 when I wrote a proposal and I hadn't a feeling

2017 is seven years ago so it would be good to get current feedback on
the desirability of this feature.

> There is one stronger argument for session variables - we are missing global
> temporary tables. It is a real
> limit and more times I found users with bloated pg_class, pg_attributes due
> using temp tables. I don't believe
> so we can have a global temp table - it is a significantly more difficult task
> than session variables. At the end
> session variables are trivial against global temp tables, and can replace
> global temp tables in some use cases.
> And the solution can be nicer, cleaner, safer than with a workaround based on
> GUC.

So this feature would be like global GUC variables, with permission
control?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Fri, Jan 17, 2025 at 04:55:07PM +0100, Pavel Stehule wrote:
> pá 17. 1. 2025 v 16:35 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> 
>     So this feature would be like global GUC variables, with permission
>     control?
> 
> + types and domain type check - holds data in binary form - there are not
> conversions binary, text
> + it is declared - so less space for misuse is there. Custom GUC are absolutely
> tolerant
> + it is a fully database object, only owner can alter it,  and event triggers
> are supported, sinval
> + possibility to set mutability, default value

Okay, good summary.  Now, can people give feedback that they would want
this committed to PostgreSQL?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Fri, Jan 17, 2025 at 11:01:41AM -0500, Bruce Momjian wrote:
> On Fri, Jan 17, 2025 at 04:55:07PM +0100, Pavel Stehule wrote:
> > pá 17. 1. 2025 v 16:35 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> > 
> >     So this feature would be like global GUC variables, with permission
> >     control?
> > 
> > + types and domain type check - holds data in binary form - there are not
> > conversions binary, text
> > + it is declared - so less space for misuse is there. Custom GUC are absolutely
> > tolerant
> > + it is a fully database object, only owner can alter it,  and event triggers
> > are supported, sinval
> > + possibility to set mutability, default value
> 
> Okay, good summary.  Now, can people give feedback that they would want
> this committed to PostgreSQL?

I unsurprisingly would really like to see it committed, for all the reasons
Pavel mentioned.  It would have helped me on various projects many times.

On Fri, 2025-01-17 at 11:01 -0500, Bruce Momjian wrote:
> On Fri, Jan 17, 2025 at 04:55:07PM +0100, Pavel Stehule wrote:
> > pá 17. 1. 2025 v 16:35 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> >
> >     So this feature would be like global GUC variables, with permission
> >     control?
> >
> > + types and domain type check - holds data in binary form - there are not
> > conversions binary, text
> > + it is declared - so less space for misuse is there. Custom GUC are absolutely
> > tolerant
> > + it is a fully database object, only owner can alter it,  and event triggers
> > are supported, sinval
> > + possibility to set mutability, default value
>
> Okay, good summary.  Now, can people give feedback that they would want
> this committed to PostgreSQL?

I would like to see this committed too, or at least relevant parts of it.

It addresses the perennial problem of people putting state into placeholder
GUCs to pass information between the application and the database
(SET myapp.application_id = 'user_laurenz').

Also, it cann pass information between the code in DO statements and
the surrounding SQL code.

Yours,
Laurenz Albe

> On Fri, Jan 17, 2025 at 11:01:41AM GMT, Bruce Momjian wrote:
> On Fri, Jan 17, 2025 at 04:55:07PM +0100, Pavel Stehule wrote:
> > pá 17. 1. 2025 v 16:35 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> >
> >     So this feature would be like global GUC variables, with permission
> >     control?
> >
> > + types and domain type check - holds data in binary form - there are not
> > conversions binary, text
> > + it is declared - so less space for misuse is there. Custom GUC are absolutely
> > tolerant
> > + it is a fully database object, only owner can alter it,  and event triggers
> > are supported, sinval
> > + possibility to set mutability, default value
>
> Okay, good summary.  Now, can people give feedback that they would want
> this committed to PostgreSQL?

+1 into the bucket "want committed" from me as well. Throughout the
review process I've stumbled upon a few cases in my own projects, where
it would be useful.

Bruce Momjian:
> Now, can people give feedback that they would want
> this committed to PostgreSQL?

 From a user's perspective: Yes!

I've been waiting for this for a long time and I really hope this can go 
through, eventually.

Best,

Wolfgang

Le 17/01/2025 à 19:01, Bruce Momjian a écrit :
> On Fri, Jan 17, 2025 at 04:55:07PM +0100, Pavel Stehule wrote:
>> pá 17. 1. 2025 v 16:35 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
>>
>>      So this feature would be like global GUC variables, with permission
>>      control?
>>
>> + types and domain type check - holds data in binary form - there are not
>> conversions binary, text
>> + it is declared - so less space for misuse is there. Custom GUC are absolutely
>> tolerant
>> + it is a fully database object, only owner can alter it,  and event triggers
>> are supported, sinval
>> + possibility to set mutability, default value
> Okay, good summary.  Now, can people give feedback that they would want
> this committed to PostgreSQL?
>

For me, this is a must have. Not only because of the huge amount of time 
that we will save in migration to PostgreSQL (i know that this is not an 
argument) but because the only way we have to emulate such feature is to 
use custom configuration directives or a PL language that allows global 
variable. I have question almost every week on how to do that and the 
only answer I have is do this ugly pl/pgsql coding. It will be nice to 
have a feature to do that. Thanks Pavel and all involved in this 
implementation.

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

On Fri, Jan 17, 2025 at 03:47:28PM +0100, Álvaro Herrera wrote:
> On 2025-Jan-17, Bruce Momjian wrote:
> 
> > Is this really something we are considering applying, since it has been
> > around for years?  I am unclear on that and we had better know if we are
> > going to continue reviewing this.
> 
> The fact that the patch has been around for years doesn't automatically
> mean it's a bad idea.

Yes, I think we passed the Desirability criteria with the feedback on
this thread, but it is now a question of whether the code complexity
justifies the feature.  I saw a few people saying they want _some_ parts
of the patch, which opens the suggestion that even people who want the
patch are seeing parts of the patch that are too much.  I have seen this
patch circling around, and I think it needs a step a back for analysis.

> I have proposed that we discuss this patch at fosdem developer's meeting
> next month, precisely to seek consensus on whether this patch is
> something we want or not.  My view is that this is a feature that has
> been requested by users for years, so IMO we want this or something
> similar.

Yes, the meeting review is a very good idea.

> I wonder if the reason that committers stay away from it is that
> reviewing it fully (and thus taking responsibility for it) seems such a
> daunting task.  I might be wrong, but I think this may be the largest
> patch since FTS.

I think we have to identify a committer who is willing to consider
application of this patch before the patch can move forward.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Mon, 2025-01-20 at 15:15 -0500, Bruce Momjian wrote:
> Yes, I think we passed the Desirability criteria with the feedback on
> this thread, but it is now a question of whether the code complexity
> justifies the feature.  I saw a few people saying they want _some_ parts
> of the patch, which opens the suggestion that even people who want the
> patch are seeing parts of the patch that are too much.  I have seen this
> patch circling around, and I think it needs a step a back for analysis.

I'd say that patches 1 to 6 from the patch set are essential.

Yours,
Laurenz Albe

hi.

git diff --check
shows there is some white space there.

Adding hack mailing list discussion links in each patch would be great.
Others said that the first 6 patches are essential, maybe we can only
post the first 6 patches.
so the test CI result would be more reliable. also people would not
feel intimidated by
the bigger patchset.


create domain dnn as int not null;
CREATE VARIABLE var3 AS dnn;
alter domain dnn drop not null;
alter domain dnn set not null;
ERROR:  cannot alter domain "dnn" because session variable "public.var3" uses it

This is not great.
we allow a constraint, then we drop it, now we cannot re add it again.
0001 and 0002 are simple, but the size is still large.
maybe we can not support the domain in the first 2 patches.

also
CREATE VARIABLE var3 AS dnn;
let var3 = 11;
create view v1 as select var3;
select * from v1;
you can see reconnect to session then
ERROR:  domain dnn does not allow null values
this is not ok?


also
create table t(var3 int);
CREATE POLICY p1r ON t AS RESTRICTIVE TO alice USING (var3 <> var3);
create table t1(a int);
CREATE POLICY p2 ON t1 AS RESTRICTIVE TO alice USING (a <> var3);
p1r is so confusing. there is no way to understand the intention.
p2 should also not be allowed, since var3 value is volatile,
session reconnection will change the value.


src/bin/pg_dump/pg_dump.h
/*
 * The VariableInfo struct is used to represent session variables
 */
typedef struct _VariableInfo
{
    DumpableObject dobj;
    DumpableAcl dacl;
    Oid            vartype;
    char       *vartypname;
    char       *varacl;
    char       *rvaracl;
    char       *initvaracl;
    char       *initrvaracl;
    Oid            varcollation;
    const char *rolname;        /* name of owner, or empty string */
} VariableInfo;
these fields (varacl, rvaracl, initvaracl, initrvaracl) were never being used.
we can remove them.

        CollInfo   *coll;
        coll = findCollationByOid(varcollation);
        if (coll)
            appendPQExpBuffer(query, " COLLATE %s",
                              fmtQualifiedDumpable(coll));
here, it should be
```CollInfo   *coll = NULL;```?


I don't understand the changes made in getAdditionalACLs.
I thought pg_init_privs had nothing to do with the session variable.


minor issue in getVariables.
    query = createPQExpBuffer();
    resetPQExpBuffer(query);
no need to use resetPQExpBuffer here.


create type ab as (a int, b text);
create type abc as (a ab, c text);
create type abcd as (a abc, d text);
CREATE VARIABLE var2 AS abcd;

select var2.a.c;
ERROR:  cross-database references are not implemented: var2.a.c

Is this error what we expected? I am not 100% sure.
--------------------another contrived corner case.-----------------------
create type pg_variable as (
  oid oid, vartype oid, varcreate_lsn pg_lsn,
  varname name, varnamespace oid, varowner oid,
  vartypmod int, varcollation oid, varacl aclitem[]);
create variable pg_variable as pg_variable;
let pg_variable = row (18041, 10116, '0/25137B0','pg_variable', 2200,
10,-1,0, NULL)::pg_variable;

select pg_variable.oid from pg_variable where pg_variable.oid = pg_variable.oid;
this query, the WHERE clause, it's really hard to distinguish session
variable or column reference.
I am not sure if this is fine or not.

On Fri, Feb 7, 2025 at 3:25 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>

Hi
The following review is based on v20250117.

pg_dump order seems not right.
CREATE FUNCTION public.test11(text) RETURNS text
    LANGUAGE sql
    AS $$select v4 $$;
CREATE VARIABLE public.v4 AS text;

first dump function then variable. restore would fail.
we should first dump variables then function.
probably placed it right after CREATE DOMAIN/CREATE TYPE


drop table if exists t3;
create variable v4 as text;
let v4 = 'hello';
CREATE TABLE t3 (a timestamp, v4 text);
INSERT INTO t3 SELECT i FROM generate_series('2020-01-01'::timestamp,
                                             '2020-12-31'::timestamp,
                                             '10 minute'::interval) s(i);
ANALYZE t3;
create or replace function test11(text) returns text as $$select v4 $$
language sql;
CREATE STATISTICS s4 (ndistinct) ON test11(v4), test11(v4 || 'h') FROM t3;
this "CREATE STATISTICS s4..." should error out?


any objects built on top of functions that use variables should be
marked as volatile.
and we should also consider the implications of it.

On Thu, May 15, 2025 at 08:48:37AM +0200, Pavel Stehule wrote:
> Hi
> 
> fresh rebase

I will again ask why this patch set is being reposted when there is no
plan to apply it to git master?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Tue, May 20, 2025 at 01:33:18PM -0300, Marcos Pegoraro wrote:
> Em ter., 20 de mai. de 2025 às 11:56, Bruce Momjian <bruce@momjian.us>
> escreveu:
> 
>     I will again ask why this patch set is being reposted when there is no
>     plan to apply it to git master
> 
> Too bad. I would love to have this functionality, from the user's point of view
> there are problems where it would solve them wonderfully. I don't know
> technically of what prevents it from being natively on core, but it would be
> great, it would definitely be.

My only point is that we should only be using email lists for work that
is being actively worked on to be added to community Postgres.  There
has been talk of a trimmed-down version of this being applied, but I
don't see any work in that direction.

This patch should be moved to a separate location where perhaps people
can subscribe to updates when they are posted, perhaps github.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

> On 20 May 2025, at 18:39, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Tue, May 20, 2025 at 01:33:18PM -0300, Marcos Pegoraro wrote:
>> Em ter., 20 de mai. de 2025 às 11:56, Bruce Momjian <bruce@momjian.us>
>> escreveu:
>>
>>    I will again ask why this patch set is being reposted when there is no
>>    plan to apply it to git master
>>
>> Too bad. I would love to have this functionality, from the user's point of view
>> there are problems where it would solve them wonderfully. I don't know
>> technically of what prevents it from being natively on core, but it would be
>> great, it would definitely be.
>
> My only point is that we should only be using email lists for work that
> is being actively worked on to be added to community Postgres.  There
> has been talk of a trimmed-down version of this being applied, but I
> don't see any work in that direction.
>
> This patch should be moved to a separate location where perhaps people
> can subscribe to updates when they are posted, perhaps github.

As a project with no roadmap governed by open forum consensus I don't think we
have any right to tell community members what they can or cannot work on here,
any technical discussion which conforms with our published policies should be
welcome.  If Pavel want's to continue rebasing his patchset here then he has,
IMHO, every right to do so.

Whether or not a committer will show interest at some point is another thing,
but we are seeing a very good role-model for taking responsibility for ones
work here at the very least =)

--
Daniel Gustafsson

On Tue, May 20, 2025 at 08:47:36PM +0200, Daniel Gustafsson wrote:
> > On 20 May 2025, at 18:39, Bruce Momjian <bruce@momjian.us> wrote:
> > My only point is that we should only be using email lists for work that
> > is being actively worked on to be added to community Postgres.  There
> > has been talk of a trimmed-down version of this being applied, but I
> > don't see any work in that direction.
> > 
> > This patch should be moved to a separate location where perhaps people
> > can subscribe to updates when they are posted, perhaps github.
> 
> As a project with no roadmap governed by open forum consensus I don't think we
> have any right to tell community members what they can or cannot work on here,
> any technical discussion which conforms with our published policies should be
> welcome.  If Pavel want's to continue rebasing his patchset here then he has,
> IMHO, every right to do so.
> 
> Whether or not a committer will show interest at some point is another thing,
> but we are seeing a very good role-model for taking responsibility for ones
> work here at the very least =)

Well, we do have a right, e.g., we would not allow someone to repeatedly
post patches for a Postgres extension we don't manage, or the jdbc
driver.  I also don't think we would allow someone to continue posting
patches for a feature we have decided to reject, and I think we have
decided to reject the patch in in its current form.  I think we might
accept a trimmed-down version, but I don't see the patch moving in that
direction.

Now, of course, if I am the only one who feels this way, I can suppress
these emails on my end.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Tue, May 20, 2025 at 10:28:31PM +0200, Pavel Stehule wrote:
> út 20. 5. 2025 v 18:39 odesílatel Bruce Momjian <bruce@momjian.us> napsal:
> I sent a reduced version a few months ago - from 21 patches to 8 (and it can be
> reduced to six if we postpone tools for detection ambiguity).
> The timing was not perfect - the focus was and it is concentrated to finish
> pg18.

It was not clear to me that this patch set was being reduced to make it
more likely it would be accepted?  I thought it was the same patch set
since 20??.

> I am very sorry if this topic and patches bother anyone. I am afraid if I close
> it to some personal github, it will not be visible, and I am sure this
> feature is missing in Postgres. Today we have few workarounds. Some workarounds
> are not available everywhere, some workarounds cannot
> be used for security. With integrated solutions some scenarios can be done more
> easily, more secure, faster, more comfortable.  It is true, so
> mentioned scenarios are not "hot" today. Stored procedures or RLS or migration
> procedures from other databases are not extra common. But
> who uses it, then he misses session variables.

Understood.  If people feel it is progressing toward acceptance, I
certainly withdraw my objection and apologize.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Tue, May 20, 2025 at 10:28:31PM +0200, Pavel Stehule wrote:
> This topic is difficult, because there is no common solution. SQL/PSM is
> almost dead. T-SQL (and MySQL) design is weak and cannot be used for
> security.
> Oracle's design is joined with just one environment. And although almost
> all widely used databases have supported session variables for decades, no
> one design
> is perfect. Proposed design is not perfect too (it introduces possible
> ambiguity) , but I think it can support most wanted use cases (can be
> enhanced in future),
> and it is consistent with Postgres. There are more ways to reduce risk of
> unwanted ambiguity to zero. But it increases the size of the patch.

One thing that I keep hearing about this feature is that this would be
really helpful for migration from Oracle to PostgreSQL, helping a lot
with rewrites of pl/pgsql functions.

There is one page on the wiki about private variables, dating back to
2016:
https://wiki.postgresql.org/wiki/CREATE_PRIVATE_VARIABLE

Perhaps it would help to summarize a bit the pros and cons of existing
implementations to drive which implementation would be the most suited
on a wiki page?  The way standards are defined is by overwriting
existing standards, and we don't have one in the SQL specification.
It's hard to say if there will be one at some point, but if the main
SQL products around the world have one, it pretty much is a point in
favor of having a standard.

Another possible angle that could be taken is to invest in a proposal
for the SQL committee to consider, forcing an actual standard that
PostgreSQL could rely on rather than having one behavior implemented
to have it standard-incompatible a few years after.  And luckily, we
have Vik Fearing and Peter Eisentraut in this community who invest
time and resources in this area.

FWIW, I do agree with the opinion that if you want to propose rebased
versions of this patch set on a periodic basis, you are free to do so.
This is the core concept of an open community.  In terms of my
committer time, I'm pretty much already booked in terms of features
planned for the next release, so I guess that I won't be able to
invest time on this thread.  Just saying.

I know that this patch set has been discussed at FOSDEM at some point,
so others may be able to comment more about that.  That's just one
opinion among many others.
--
Michael

On Wed, May 21, 2025 at 09:12:54AM +0200, Pavel Stehule wrote:
> Last discussion is related to reducing the size of the session variable patch
> set.
> 
> I have an idea to use variable's fencing more aggressively from the start, and
> then we can reduce it in future. This should not break issues with
> compatibility and doesn't need some like version flags.
> 
> The real problem of proposed session variables is possible collisions between
> session variables identifiers and table or columns identifiers. I designed some
> tools to minimize the risk of unwanted collisions, but these tools increase the
> size of code and don't reduce the complexity of the patch and tests. The
> proposed change probably doesn't reduce a lot of code, but can reduce some
> tests, and mainly possible risk of some unwanted impact - at the end it can be
> less work for reviewers and less stress for committers - and the implementation
> can be divided to allone workable following steps.

Yes, I remember the discussions about how the creation of server
variables could break existing queries.  Our scoping rules are already
complex, so adding another scope would add a lot of complexity.

> Step 1
> =====
> 
> So the main change is the hard requirement for usage variable's fence
> everywhere where collisions are possible - and then in the first step, the
> collisions will not be possible, and then we don't need it to solve, and we
> don't need to test it.
> 
> CREATE VARIABLE public.foo AS int;
> LET foo = 10;
> SELECT VARIABLE(foo);

Yes, I can see how adding fencing like VARIABLE() would simplify things.

> Step 2
> =====
> Necessity of usage variable fencing in PL/pgSQL can be a problem for migration
> from PL/SQL. But this can be solved separately by using SPI params hooks -
> similar to how PL/pgSQL works with PL/pgSQL variables. In this step we can push
> optimization for fast execution of the LET statement or optimization of usage
> variables in queries.

Yes, there is already going to be migration requirements in moving from
PL/SQL to PL/pgSQL, so the requirement to add VARIABLE() seems minimal.

> After this step will be possible:
> 
> DO $$
> BEGIN
>   RAISE NOTICE '% %', foo, VARIABLE(public.foo);
> END;
> $$;
> 
> SELECT VARIABLE(foo);
> 
> No other visible change in this step. WIth this step the people who do
> migration form Oracle and PL/pgSQL developers will be very happy. They don't
> need more. There can be collisions, but the collisions can be limited just to
> PL/pgSQL scope, and we can use already implemented mechanisms.
> 
> Step 3
> =====
> We can talk in future about less requirement of usage variable fencing in
> queries. This needs to introduce some form of detection collisions and how they
> should be solved (outside PL/pgSQL).
> We can talk about other features like temporal, default values, transactional,
> etc ...

I feel that if we haven't found a good solution to this in 13 years, we
should assume it is unsolvable and just accept an imperfect solution.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Wed, May 21, 2025 at 07:49:34AM +0200, Pavel Stehule wrote:
> st 21. 5. 2025 v 2:21 odesílatel Michael Paquier <michael@paquier.xyz> napsal:
>     One thing that I keep hearing about this feature is that this would be
>     really helpful for migration from Oracle to PostgreSQL, helping a lot
>     with rewrites of pl/pgsql functions.
> 
>     There is one page on the wiki about private variables, dating back to
>     2016:
>     https://wiki.postgresql.org/wiki/CREATE_PRIVATE_VARIABLE
> 
> I wrote mail 
> 
> https://www.postgresql.org/message-id/
> CAFj8pRB8kdWQCdN2X1_63c58+07Oy4Z+ruDK_xPTUP+Pe8R2Pw@mail.gmail.com
> 
> and there is another wiki page https://wiki.postgresql.org/wiki/Variable_Design

Yes, these URLs are very helpful, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

Hi

On 9/13/25 11:28, Pavel Stehule wrote:
> 
> minor change (after private talk with Jim Jones)
> 

After another pass, there are a few additional tests that should be
included in this patch:


== Additional tests for GRANT x ON VARIABLE ==
==============================================

We wanna make sure the proper error message is raised.

postgres=# GRANT INSERT ON VARIABLE var TO jim;
ERROR:  invalid privilege type INSERT for session variable

postgres=# GRANT DELETE ON VARIABLE var TO jim;
ERROR:  invalid privilege type DELETE for session variable


== Tests for ALTER DEFAULT PRIVILEGES ==
========================================

I couldn't find regression tests for ALTER DEFAULT PRIVILEGES ... if I
haven't just missed them, I think they'd be a nice addition.

postgres=# ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT UPDATE ON
VARIABLES TO jim;
ALTER DEFAULT PRIVILEGES

postgres=# ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT INSERT ON
VARIABLES TO jim;
ERROR:  invalid privilege type INSERT for session variable

postgres=# ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT DELETE ON
VARIABLES TO jim;
ERROR:  invalid privilege type DELETE for session variable

postgres=# ALTER DEFAULT PRIVILEGES IN SCHEMA s GRANT SELECT ON
VARIABLES TO jim;
ALTER DEFAULT PRIVILEGES


Best, Jim