MIME-Version: 1.0
References: 
 <CAFj8pRC+hPCc2X88xC=pTJoqmVPApDsageZOMyqaxi5788WxHA@mail.gmail.com>
 <CAFj8pRDJ9cq00VYSHxs6LsoHNWjhYXyWWBtV6UgeWwhs0AHa9A@mail.gmail.com>
 <CAFj8pRBPXTcw_3fpKtgVthV2+9rZGhxitZ40DnAwCrK601TZZg@mail.gmail.com>
 <ndtfl4tsnpkb7m7hwvnmlpsascpgd3a7xvjmjhtxffsbrgygtm@4du6zsmnnwq5>
 <CAFj8pRAu4XvNCGu1751t=2YEqLqTjDA3FavMExm2S0KYQq=DdQ@mail.gmail.com>
 <CAFj8pRAsEoeZv0HEnA8CKgFKDSQ-wYw18Os1vdksWCV7ez2bVw@mail.gmail.com>
 <3chredgnjcmccym2kczawfih226b4ac6co7p6z4jeofevrcosi@mrsxkx2x2c65>
 <CAFj8pRBoWPDTOwn5FmMzc+1qiopw+N04U26nviOdF61fs8A2wQ@mail.gmail.com>
 <stckyvkl4yyzvgjsaawojs3xikke7mmds5bhv7l7qerclywywk@h4v4n43xm6u2>
 <CAFj8pRB_E1GM_YGT-ti4bXka6mhLdAAFeTe+BHgHFYC+qb-76g@mail.gmail.com>
 <20241120201313.t4wbhld4ktgielaf@erthalion.local>
 <CAFj8pRBWqEb8i6WmrF_Xh64=48GtisKijgczMv7HTTpe4GswuA@mail.gmail.com>
 <CAFj8pRAry0esQiHcK=6BwwFKDY0zanug6k07CEQzRPBqZ6iW0Q@mail.gmail.com>
 <CACJufxFNjKrmyEi9SLfPCq4c9GUN+5eoOtbZwBPq9eKoO8REUw@mail.gmail.com>
 <CAFj8pRALQ-j-Dz3R1ivCoXut8LEhN+kSa7U8Gshucdv5zU3AfQ@mail.gmail.com>
In-Reply-To: 
 <CAFj8pRALQ-j-Dz3R1ivCoXut8LEhN+kSa7U8Gshucdv5zU3AfQ@mail.gmail.com>
From: jian he <jian.universality@gmail.com>
Date: Fri, 20 Dec 2024 15:57:33 +0800
Message-ID: 
 <CACJufxG7LvaNbF8ZSCcxOVUbm9W=KGjD=h_Wk+5imMw4s_2QxA@mail.gmail.com>
Subject: Re: Re: proposal: schema variables
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Dmitry Dolgov <9erthalion6@gmail.com>,
 Laurenz Albe <laurenz.albe@cybertec.at>,
	Erik Rijkers <er@xs4all.nl>, Michael Paquier <michael@paquier.xyz>,
	Amit Kapila <amit.kapila16@gmail.com>, DUVAL REMI <REMI.DUVAL@cheops.fr>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxG7LvaNbF8ZSCcxOVUbm9W%3DKGjD%3Dh_Wk%2B5imMw4s_2QxA%40mail.gmail.com>
Precedence: bulk

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?