Re: Re: proposal: schema variables - Mailing list pgsql-hackers

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?