From d3d3ea03741b730e3f91c0e0402c5fb2870b8aa8 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Wed, 3 Nov 2021 14:05:02 +0100 Subject: [PATCH 2/3] Schema variables - new implementation for Postgres 15 Hi, I took a quick look at the latest patch version. In general the patch ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company --- doc/src/sgml/advanced.sgml | 27 ++++----- doc/src/sgml/catalogs.sgml | 8 +-- doc/src/sgml/plpgsql.sgml | 2 +- doc/src/sgml/ref/discard.sgml | 2 +- src/backend/catalog/aclchk.c | 4 +- src/backend/catalog/dependency.c | 2 +- src/backend/catalog/namespace.c | 6 +- src/backend/commands/schemavariable.c | 81 ++++++++++++++++++--------- src/backend/nodes/copyfuncs.c | 2 +- src/backend/optimizer/util/clauses.c | 4 +- src/backend/parser/analyze.c | 4 ++ src/backend/parser/gram.y | 1 + src/backend/parser/parse_expr.c | 4 +- src/bin/pg_dump/pg_dump.c | 2 +- 14 files changed, 96 insertions(+), 53 deletions(-) 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 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 GRANT and REVOKE commands. - The schema variable can be created by the CREATE VARIABLE command. + A schema variable can be created by the CREATE VARIABLE command. - 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. - - - - Schema variables are retrieved by the SELECT SQL command. - Their value is set with the LET SQL command. + The value of a schema variable is set with the LET SQL command. While schema variables share properties with tables, their value cannot be updated - with an UPDATE command. + with an UPDATE command. The value of a schema variable may be + retrieved by the SELECT SQL command. 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; + + + 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 using the + LET command. The content of a variable is not transactional. + This is the same as regular variables in PL languages. + + diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index db0d2fe859..4df06148c3 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -13995,8 +13995,8 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx - The table pg_variable holds metadata - of schema variables. + The table pg_variable provides information + about schema variables. @@ -14052,8 +14052,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx vartypmod records type-specific data supplied at variable creation time (for example, the maximum length of a varchar 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 vartypmod. @@ -14098,6 +14097,7 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx char + Action performed at end of transaction: n = no action, d = drop the variable, r = reset the variable to its default value. 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; - <command>Global variables and constants</command> + <command>Schema variables and constants</command> The PL/pgSQL 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 } Resets the value of all schema variables. If a variable �����is later reused, it is re-initialized to either - �����NULL or its default value. + �����NULL or its default value. 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(); -- 2.17.0