Hi,
I took a quick look at the latest patch version. In general the patch
looks pretty complete and clean, and for now I have only some basic
comments. The attached patch tweaks some of this, along with a couple
additional minor changes that I'll not discuss here.
1) Not sure why we need to call this "schema variables". Most objects
are placed in a schema, and we don't say "schema tables" for example.
And it's CREATE VARIABLE and not CREATE SCHEMA VARIABLE, so it's a bit
inconsistent.
The docs actually use "Global variables" in one place for some reason.
2) I find this a bit confusing:
SELECT non_existent_variable;
test=# select s;
ERROR: column "non_existent_variable" does not exist
LINE 1: select non_existent_variable;
I wonder if this means using SELECT to read variables is a bad idea, and
we should have a separate command, just like we have LET (instead of
just using UPDATE in some way).
3) I've reworded / tweaked a couple places in the docs, but this really
needs a native speaker - I don't have a very good "feeling" for this
technical language so it's probably still quite cumbersome.
4) Is sequential scan of the hash table in clean_cache_callback() a
good idea? I wonder how fast (with how many variables) it'll become
noticeable, but it may be good enough for now and we can add something
better (tracing which variables need resetting) later.
5) In what situation would we call clean_cache_callback() without a
transaction state? If that happens it seems more like a bug, so
maybeelog(ERROR) or Assert() would be more appropriate?
6) free_schema_variable does not actually use the force parameter
7) The target_exprkind expression in transformSelectStmt really needs
some explanation. Because that's chance you'll look at this in 6 months
and understand what it does?
target_exprkind =
(pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
pstate->parentParseState != NULL) ?
EXPR_KIND_SELECT_TARGET : EXPR_KIND_LET_TARGET;
8) immutable variables without a default value
IMO this case should not be allowed. On 2021/08/29 you wrote:
I thought about this case, and I have one scenario, where this
behaviour can be useful. When the variable is declared as IMMUTABLE
NOT NULL without not null default, then any access to the content of
the variable has to fail. I think it can be used for detection,
where and when the variable is first used. So this behavior is
allowed just because I think, so this feature can be interesting for
debugging. If this idea is too strange, I have no problem to disable
this case.
This seems like a really strange use case. In a production code you'll
not do this, because then the variable is useless and the code does not
work at all (it'll just fail whenever it attempts to access the var).
And if you can modify the code, there are other / better ways to do this
(raising an exception, ...).
So this seems pretty useless to me, +1 to disabling it.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml
index 2d6555497c..bd4733c0bb 100644
--- a/doc/src/sgml/advanced.sgml
+++ b/doc/src/sgml/advanced.sgml
@@ -714,24 +714,16 @@ SELECT name, elevation
<para>
Schema variables are database objects that can hold a value.
- Schema variables, like relations, exist within a schema and their access is
+ Schema variables, like relations, exist within a schema and the access is
controlled via <command>GRANT</command> and <command>REVOKE</command> commands.
- The schema variable can be created by the <command>CREATE VARIABLE</command> command.
+ A schema variable can be created by the <command>CREATE VARIABLE</command> command.
</para>
<para>
- The value of a schema variable is local to the current session. Retrieving
- a variable's value returns either a NULL or a default value, unless its value
- is set to something else in the current session with a LET command. The content
- of a variable is not transactional. This is the same as in regular variables
- in PL languages.
- </para>
-
- <para>
- Schema variables are retrieved by the <command>SELECT</command> SQL command.
- Their value is set with the <command>LET</command> SQL command.
+ The value of a schema variable is set with the <command>LET</command> SQL command.
While schema variables share properties with tables, their value cannot be updated
- with an <command>UPDATE</command> command.
+ with an <command>UPDATE</command> command. The value of a schema variable may be
+ retrieved by the <command>SELECT</command> SQL command.
<programlisting>
CREATE VARIABLE var1 AS date;
LET var1 = current_date;
@@ -747,6 +739,15 @@ LET current_user_id = (SELECT id FROM users WHERE usename = session_user);
SELECT current_user_id;
</programlisting>
</para>
+
+ <para>
+ The value of a schema variable is local to the current session. Retrieving
+ a variable's value returns either a <literal>NULL</literal> or a default value,
+ unless its value is set to something else in the current session using the
+ <command>LET</command> command. The content of a variable is not transactional.
+ This is the same as regular variables in PL languages.
+ </para>
+
</sect1>
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 179fa873dc..400724456f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -13972,8 +13972,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
</indexterm>
<para>
- The table <structname>pg_variable</structname> holds metadata
- of schema variables.
+ The table <structname>pg_variable</structname> provides information
+ about schema variables.
</para>
<table>
@@ -14029,8 +14029,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<structfield>vartypmod</structfield> records type-specific data
supplied at variable creation time (for example, the maximum
length of a <type>varchar</type> column). It is passed to
- type-specific input
- functions and length coercion functions.
+ type-specific input functions and length coercion functions.
The value will generally be -1 for types that do not need <structfield>vartypmod</structfield>.
</entry>
</row>
@@ -14075,6 +14074,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
<entry><type>char</type></entry>
<entry></entry>
<entry>
+ Action performed at end of transaction:
<literal>n</literal> = no action, <literal>d</literal> = drop the variable,
<literal>r</literal> = reset the variable to its default value.
</entry>
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index e494161741..fb2ef96d0d 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5954,7 +5954,7 @@ $$ LANGUAGE plpgsql STRICT IMMUTABLE;
</sect3>
<sect3>
- <title><command>Global variables and constants</command></title>
+ <title><command>Schema variables and constants</command></title>
<para>
The <application>PL/pgSQL</application> language has no packages,
diff --git a/doc/src/sgml/ref/discard.sgml b/doc/src/sgml/ref/discard.sgml
index 698b2c07fd..7d6ad2dd49 100644
--- a/doc/src/sgml/ref/discard.sgml
+++ b/doc/src/sgml/ref/discard.sgml
@@ -81,7 +81,7 @@ DISCARD { ALL | PLANS | SEQUENCES | TEMPORARY | TEMP | VARIABLES }
<para>
Resets the value of all schema variables. If a variable
is later reused, it is re-initialized to either
- NULL or its default value.
+ <literal>NULL</literal> or its default value.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 8d1c02b871..715208af23 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4763,7 +4763,7 @@ pg_variable_aclmask(Oid var_oid, Oid roleid, AclMode mask, AclMaskHow how)
return mask;
/*
- * Must get the type's tuple from pg_type
+ * Must get the variables's tuple from pg_variable
*/
tuple = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(var_oid));
if (!HeapTupleIsValid(tuple))
@@ -4774,7 +4774,7 @@ pg_variable_aclmask(Oid var_oid, Oid roleid, AclMode mask, AclMaskHow how)
varForm = (Form_pg_variable) GETSTRUCT(tuple);
/*
- * Now get the type's owner and ACL from the tuple
+ * Now get the variable's owner and ACL from the tuple
*/
ownerId = varForm->varowner;
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 4ec76d4e03..f504701515 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1878,8 +1878,8 @@ find_expr_references_walker(Node *node,
{
Param *param = (Param *) node;
+ /* A variable parameter depends on the schema variable */
if (param->paramkind == PARAM_VARIABLE)
- /* A variable parameter depends on the schema variable */
add_object_address(OCLASS_VARIABLE, param->paramvarid, 0,
context->addrs);
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 02c5de7f73..789f41876c 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -828,7 +828,6 @@ VariableIsVisible(Oid varid)
return visible;
}
-
/*
* TypenameGetTypid
* Wrapper for binary compatibility.
@@ -2909,6 +2908,8 @@ TSConfigIsVisible(Oid cfgid)
/*
* When we know a variable name, then we can find variable simply
+ *
+ * FIXME seems incomplete
*/
Oid
LookupVariable(const char *nspname, const char *varname, bool missing_ok)
@@ -2995,6 +2996,9 @@ NamesFromList(List *names)
* Returns oid of not ambigonuous variable specified by qualified path
* or InvalidOid. When the path is ambigonuous, then not_uniq flag is
* is true.
+ *
+ * XXX Do we need the not_uniq flag, when we return InvalidOid anyway?
+ * XXX Maybe use not_unique, it's not much longer.
*/
Oid
IdentifyVariable(List *names, char **attrname, bool *not_uniq)
diff --git a/src/backend/commands/schemavariable.c b/src/backend/commands/schemavariable.c
index 15e0567199..5426e5fa21 100644
--- a/src/backend/commands/schemavariable.c
+++ b/src/backend/commands/schemavariable.c
@@ -64,6 +64,10 @@ static List *on_commits = NIL;
* implementation of DROP can be simple, because although DROP VARIABLE
* can be reverted, the content of variable can be lost. In this example,
* DROP VARIABLE is same like reset variable.
+ *
+ * XXX Seems misplaced? There is no example here.
+ * XXX Also, is it really true that discarding the value even if DROP
+ * VARIABLE gets reverted makes sense?
*/
typedef struct SchemaVariableData
@@ -108,6 +112,9 @@ static void clean_cache_variable(Oid varid);
* Save info about necessity to clean hash table, because some
* schema variable was dropped. Don't do here more, recheck
* needs to be in transaction state.
+ *
+ * XXX Not sure "needs to be in transaction state" means? Perhaps
+ * we can do the cleanup only at transaction end?
*/
static void
InvalidateSchemaVariableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
@@ -126,41 +133,53 @@ InvalidateSchemaVariableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
static void
clean_cache_callback(XactEvent event, void *arg)
{
+ HASH_SEQ_STATUS status;
+ SchemaVariable svar;
+
+ /* If no cleanup needed, we're done. */
+ if (!clean_cache_req)
+ return;
+
+ /* XXX How can this happen after having clean_cache_req=true? */
+ if (!schemavarhashtab)
+ return;
+
/*
- * should continue only in transaction time, when syscache is available.
+ * XXX Can we even get here when not in a transaction? Isn't that really
+ * an error? Maybe should be assert or elog(ERROR).
*/
- if (clean_cache_req && IsTransactionState())
- {
- HASH_SEQ_STATUS status;
- SchemaVariable svar;
+ if (!IsTransactionState())
+ return;
- if (!schemavarhashtab)
- return;
+ hash_seq_init(&status, schemavarhashtab);
- hash_seq_init(&status, schemavarhashtab);
+ /*
+ * Walk the current contents of the hash table, and check if the
+ * schema variable still exists in the catalog. If not, remove it.
+ *
+ * XXX Try using a variable still used by another session.
+ */
+ while ((svar = (SchemaVariable) hash_seq_search(&status)) != NULL)
+ {
+ HeapTuple tp;
- while ((svar = (SchemaVariable) hash_seq_search(&status)) != NULL)
+ tp = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(svar->varid));
+ if (!HeapTupleIsValid(tp))
{
- HeapTuple tp;
+ elog(DEBUG1, "variable %d is removed from cache", svar->varid);
- tp = SearchSysCache1(VARIABLEOID, ObjectIdGetDatum(svar->varid));
- if (!HeapTupleIsValid(tp))
- {
- elog(DEBUG1, "variable %d is removed from cache", svar->varid);
+ free_schema_variable(svar, true);
- free_schema_variable(svar, true);
+ remove_variable_on_commit_actions(svar->varid, VARIABLE_EOX_DROP);
- remove_variable_on_commit_actions(svar->varid, VARIABLE_EOX_DROP);
-
- (void) hash_search(schemavarhashtab, (void *) &svar->varid,
- HASH_REMOVE, NULL);
- }
- else
- ReleaseSysCache(tp);
+ (void) hash_search(schemavarhashtab, (void *) &svar->varid,
+ HASH_REMOVE, NULL);
}
-
- clean_cache_req = false;
+ else
+ ReleaseSysCache(tp);
}
+
+ clean_cache_req = false;
}
/*
@@ -202,12 +221,19 @@ create_schema_variable_hashtable(void)
}
/*
- * Fast drop complete content of schema variables
+ * Fast drop complete content of schema variables hash table.
*/
void
ResetSchemaVariableCache(void)
{
- /* Remove temporal schema variables */
+ /*
+ * Remove temporary schema variables.
+ *
+ * XXX This does not *remove* the variables, does it? If the variable
+ * used ON COMMIT RESET then it'll still be there.
+ *
+ * XXX Is it correct to call this with "isCommit=true"?
+ */
AtPreEOXact_SchemaVariable_on_commit_actions(true);
/* Destroy hash table and reset related memory context */
@@ -228,6 +254,9 @@ ResetSchemaVariableCache(void)
* current value for possible future usage on rollback.
*
* When force is true, then release current and possibly archived value.
+ *
+ * XXX The "force" flag is not really used.
+ * XXX Can't the value be something more complex?
*/
static void
free_schema_variable(SchemaVariable svar, bool force)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 44e42c6f69..5922e6fc34 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -5955,7 +5955,7 @@ copyObjectImpl(const void *from)
break;
default:
- elog(ERROR, "urecognized node type: %d", (int) nodeTag(from));
+ elog(ERROR, "unrecognized node type: %d", (int) nodeTag(from));
retval = 0; /* keep compiler quiet */
break;
}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b18528ffbc..30d9662148 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4761,7 +4761,9 @@ substitute_actual_parameters_mutator(Node *node,
{
if (node == NULL)
return NULL;
- if (IsA(node, Param) &&((Param *) node)->paramkind != PARAM_VARIABLE)
+
+ /* XXX Explain why this does not work with PARAM_VARIABLE? */
+ if (IsA(node, Param) && ((Param *) node)->paramkind != PARAM_VARIABLE)
{
Param *param = (Param *) node;
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 8971cf7e80..34cbaa9100 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1290,6 +1290,10 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
* Transform targetlist. Second usage of this transformation
* is for Let statement. In this case we would to allow DEFAULT
* keyword - we specify EXPR_KIND_LET_TARGET.
+ *
+ * XXX This is really hard to understand. What if p_expr_kind is
+ * EXPR_KIND_LET_TARGET, why should it matter if parentParseState
+ * is NULL?
*/
target_exprkind =
(pstate->p_expr_kind != EXPR_KIND_LET_TARGET ||
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index fb5391d173..b56ff4f69d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4717,6 +4717,7 @@ OptSchemaVarDefExpr: DEFAULT b_expr { $$ = $2; }
| /* EMPTY */ { $$ = NULL; }
;
+/* XXX This seems a bit strange. */
OnEOXActionOption: ON COMMIT DROP { $$ = VARIABLE_EOX_DROP; }
| ON TRANSACTION END_P RESET { $$ = VARIABLE_EOX_RESET; }
| /*EMPTY*/ { $$ = VARIABLE_EOX_NOOP; }
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 6aaaa5bb29..31b5b55a09 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -281,7 +281,7 @@ transformExprRecurse(ParseState *pstate, Node *expr)
* processed it rather than passing it to transformExpr().
*/
case T_SetToDefault:
- Assert(false);
+ Assert(false); /* FIXME left here by accident? */
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("DEFAULT is not allowed in this context"),
@@ -772,6 +772,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
break;
}
+ /* XXX Why is this before the p_post_columnref_hook block? Why not as part of
+ * the varid block right after it? */
varid = IdentifyVariable(cref->fields, &attrname, ¬_unique);
if (not_unique)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index a8b727898b..5f1b386b55 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4735,7 +4735,7 @@ getVariables(Archive *fout)
int i,
ntups;
- if (fout->remoteVersion < 130000)
+ if (fout->remoteVersion < 150000)
return;
acl_subquery = createPQExpBuffer();
