public inbox for [email protected]
help / color / mirror / Atom feedFrom: jian he <[email protected]>
To: Pavel Stehule <[email protected]>
Cc: Dmitry Dolgov <[email protected]>
Cc: Laurenz Albe <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: DUVAL REMI <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: Re: proposal: schema variables
Date: Fri, 20 Dec 2024 15:57:33 +0800
Message-ID: <CACJufxG7LvaNbF8ZSCcxOVUbm9W=KGjD=h_Wk+5imMw4s_2QxA@mail.gmail.com> (raw)
In-Reply-To: <CAFj8pRALQ-j-Dz3R1ivCoXut8LEhN+kSa7U8Gshucdv5zU3AfQ@mail.gmail.com>
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>
<[email protected]>
<CAFj8pRBWqEb8i6WmrF_Xh64=48GtisKijgczMv7HTTpe4GswuA@mail.gmail.com>
<CAFj8pRAry0esQiHcK=6BwwFKDY0zanug6k07CEQzRPBqZ6iW0Q@mail.gmail.com>
<CACJufxFNjKrmyEi9SLfPCq4c9GUN+5eoOtbZwBPq9eKoO8REUw@mail.gmail.com>
<CAFj8pRALQ-j-Dz3R1ivCoXut8LEhN+kSa7U8Gshucdv5zU3AfQ@mail.gmail.com>
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?
view thread (439+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Re: proposal: schema variables
In-Reply-To: <CACJufxG7LvaNbF8ZSCcxOVUbm9W=KGjD=h_Wk+5imMw4s_2QxA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox