Hi,
On Wed, Mar 02, 2022 at 06:03:06AM +0100, Pavel Stehule wrote:
>
> I lost commit with this change. I am sending updated patch.
Thanks a lot Pavel!
I did a more thorough review of the patch. I'm attaching a diff (in .txt
extension) for comment improvement suggestions. I may have misunderstood
things so feel free to discard some of it. I will mention the comment I didn't
understand in this mail.
First, I spotted some problem in the invalidation logic.
+ * Assign sinval mark to session variable. This mark probably
+ * signalized, so the session variable was dropped. But this
+ * should be rechecked later against system catalog.
+ */
+static void
+pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
You mention that hashvalue can only be zero for commands that can't
affect session variables (like VACUUM or ANALYZE), but that's not true. It can
also happen in case of sinval queue overflow (see InvalidateSystemCaches()).
So in that case we should trigger a full recheck, with some heuristics on how
to detect that a cached variable is still valid. Unfortunately the oid can
wraparound so some other check is needed to make it safe.
Also, even if we get a non-zero hashvalue in the inval callback, we can't
assume that there weren't any collision in the hash. So the additional check
should be used there too.
We had a long off-line discussion about this with Pavel yesterday on what
heuristic to use there. Unlike other caches where discarding an entry when it
shouldn't have been is not really problematic, the cache here contains the real
variable value so we can't discard it unless the variable was really dropped.
It should be possible to make it work, so I will let Pavel comment on which
approach he wants to use and what the drawbacks are. I guess that this will be
the most critical part of this patch to decide whether the approach is
acceptable or not.
The rest is only minor stylistic comments.
Using -DRAW_EXPRESSION_COVERAGE_TEST I see that T_LetStmt is missing in
raw_expression_tree_walker.
ALTER and DROP both suggest "IMMUTABLE VARIABLE" as valid completion, while
it should only be usable in the CREATE [ IMMUTABLE ] VARIABLE form.
+initVariable(Variable *var, Oid varid, bool fast_only)
+{
+ var->collation = varform->varcollation;
+ var->eoxaction = varform->vareoxaction;
+ var->is_not_null = varform->varisnotnull;
+ var->is_immutable = varform->varisimmutable;
nit: eoxaction is defined after is_not_null and is_immutable, it would be
better to keep the initialization order consistent (same in VariableCreate).
+ values[Anum_pg_variable_varcollation - 1] = ObjectIdGetDatum((char)
varCollation);
+ values[Anum_pg_variable_vareoxaction - 1] = CharGetDatum(eoxaction);
seems like the char cast is on the wrong variable?
+ * [...] We have to hold two separate action lists:
+ * one for dropping the session variable from system catalog, and
+ * another one for resetting its value. Both are necessary, since
+ * dropping a session variable also needs to enforce a reset of
+ * the value.
I don't fully understand that comment. Maybe you meant that the opposite isn't
true, ie. highlight that a reset should *not* drop the variable thus two lists?
+typedef enum SVariableXActAction
+{
+ SVAR_ON_COMMIT_DROP, /* used for ON COMMIT DROP */
+ SVAR_ON_COMMIT_RESET, /* used for DROP VARIABLE */
+ SVAR_RESET, /* used for ON TRANSACTION END RESET */
+ SVAR_RECHECK /* verify if session variable still exists */
+} SVariableXActAction;
+
+typedef struct SVariableXActActionItem
+{
+ Oid varid; /* varid of session variable */
+ SVariableXActAction action; /* reset or drop */
the stored action isn't simply "reset or drop", even though the resulting
action will be a reset or a drop (or a no-op) right? Since it's storing a enum
define just before, I'd just drop the comment on action, and maybe specify that
SVAR_RECHECK will do appropriate cleanup if the session variable doesn't exist.
+ * Release the variable defined by varid from sessionvars
+ * hashtab.
+ */
+static void
+free_session_variable(SVariable svar)
The function name is a bit confusing given the previous function. Maybe this
one should be called forget_session_variable() instead, or something like that?
I think the function comment should also mention that caller is responsible for
making sure that the sessionvars htab exists before calling it, for extra
clarity, or just add an assert for that.
+static void
+free_session_variable_varid(Oid varid)
Similary, maybe renaming this function forget_session_variable_by_id()?
+static void
+create_sessionvars_hashtable(void)
+{
+ HASHCTL ctl;
+
+ /* set callbacks */
+ if (first_time)
+ {
+ /* Read sinval messages */
+ CacheRegisterSyscacheCallback(VARIABLEOID,
+ pg_variable_cache_callback,
+ (Datum) 0);
+
+ first_time = false;
+ }
+
+ /* needs its own long lived memory context */
+ if (SVariableMemoryContext == NULL)
+ {
+ SVariableMemoryContext =
+ AllocSetContextCreate(TopMemoryContext,
+ "session variables",
+ ALLOCSET_START_SMALL_SIZES);
+ }
As far as I can see the SVariableMemoryContext can be reset but never set to
NULL, so I think the initialization can be done in the first_time case, and
otherwise asserted that it's not NULL.
+ if (!isnull && svar->typid != typid)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("type \"%s\" of assigned value is different than type
\"%s\" of session variable \"%s.%
Why testing isnull? I don't think it's ok to allow NULL::text in an int
variable for instance. This isn't valid in other context (like inserting in a
table)
+ * result of default expression always). Don't do this check, when variable
+ * is initialized.
+ */
+ if (!init_mode &&
I think the last part of the comment is a bit misleading. Maybe "when variable
is being initialized" (and similary same for the function comment).
+ * We try not to break the previous value, if something is wrong.
+ *
+ * As side efect this function acquires AccessShareLock on
+ * related session variable until commit.
+ */
+void
+SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
I don't understand what you mean by "We try not to break the previous value, if
something is wrong".
+ /* Initialize svar when not initialized or when stored value is null */
+ if (!found)
+ {
+ Variable var;
+
+ /* don't need defexpr and acl here */
+ initVariable(&var, varid, true);
+ init_session_variable(svar, &var);
+ }
+
+ set_session_variable(svar, value, isNull, typid, false);
Shouldn't the comment be on the set_session_variable() vall rather than on the
!found block?
+ * Returns the value of the session variable specified by varid. Check correct
+ * result type. Optionally the result can be copied.
+ */
+Datum
+GetSessionVariable(Oid varid, bool *isNull, Oid expected_typid, bool copy)
All callers use copy == true, couldn't we get rid of it and say it returns a
copy of the value if any?
+ * Create new ON_COMMIT_DROP xact action. We have to drop
+ * ON COMMIT DROP variable, although this variable should not
+ * be used. So we need to register this action in CREATE VARIABLE
+ * time.
I don't understand this comment.
+AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
+{
+ ListCell *l;
+
+ foreach(l, xact_drop_actions)
+ {
+ SVariableXActActionItem *xact_ai =
+ (SVariableXActActionItem *) lfirst(l);
+
+ /* Iterate only over non dropped entries */
+ if (xact_ai->deleting_subid == InvalidSubTransactionId)
+ {
+ Assert(xact_ai->action == SVAR_ON_COMMIT_DROP);
The assert sould probably be in the block above.
+ * We want to reset session variable (release it from
+ * local memory) when RESET is required or when session
+ * variable was removed explicitly (DROP VARIABLE) or
+ * implicitly (ON COMMIT DROP). Explicit releasing should
+ * be done only if the transaction is commited.
+ */
+ if ((xact_ai->action == SVAR_RESET) ||
+ (xact_ai->action == SVAR_ON_COMMIT_RESET &&
+ xact_ai->deleting_subid == InvalidSubTransactionId &&
+ isCommit))
+ free_session_variable_varid(xact_ai->varid);
This chunk is a bit hard to follow. Also, for SVAR_RESET wouldn't it be better
to only make the svar invalid and keep it in the htab? If so, this could be
split in two different branches which would be easier to follow.
+ if (!isCommit &&
+ xact_ai->creating_subid == mySubid &&
+ xact_ai->action != SVAR_RESET &&
+ xact_ai->action != SVAR_RECHECK)
+ {
+ /* cur_item must be removed */
+ xact_reset_actions = foreach_delete_current(xact_reset_actions,
cur_item);
+ pfree(xact_ai);
I think that be definition only the SVAR_ON_COMMIT_DROP (cleaning entry for a
dropped session variable) will ever need to be removed there, so we should
check for that instead of not being something else?
+ /*
+ * Prepare session variables, if not prepared in queryDesc
+ */
+ if (queryDesc->num_session_variables > 0)
I don't understand that comment.
+static void
+svariableStartupReceiver(DestReceiver *self, int operation, TupleDesc typeinfo)
+{
+ svariableState *myState = (svariableState *) self;
+ int natts = typeinfo->natts;
+ int outcols = 0;
+ int i;
+
+ for (i = 0; i < natts; i++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(typeinfo, i);
+
+ if (attr->attisdropped)
+ continue;
+
+ if (++outcols > 1)
+ elog(ERROR, "svariable DestReceiver can take only one attribute");
+
+ myState->typid = attr->atttypid;
+ myState->typmod = attr->atttypmod;
+ myState->typlen = attr->attlen;
+ myState->slot_offset = i;
+ }
+
+ myState->rows = 0;
+}
Maybe add an initial Assert to make sure that caller did call
SetVariableDestReceiverParams(), and final check that one attribute was found?
@@ -1794,15 +1840,39 @@ fix_expr_common(PlannerInfo *root, Node *node)
g->cols = cols;
}
}
+ else if (IsA(node, Param))
+ {
+ Param *p = (Param *) node;
+
+ if (p->paramkind == PARAM_VARIABLE)
+ {
+ PlanInvalItem *inval_item = makeNode(PlanInvalItem);
+
+ /* paramid is still session variable id */
+ inval_item->cacheId = VARIABLEOID;
+ inval_item->hashValue = GetSysCacheHashValue1(VARIABLEOID,
+
ObjectIdGetDatum(p->paramvarid));
+
+ /* Append this variable to global, register dependency */
+ root->glob->invalItems = lappend(root->glob->invalItems,
+ inval_item);
+ }
+ }
I didn't see any test covering invalidation of cached plan using session
variables. Could you add some? While at it, maybe use different values on the
sesssion_variable.sql tests rather than 100 in many places, so it's easier to
identifier which case broke in case of problem.
+static Node *
+makeParamSessionVariable(ParseState *pstate,
+ Oid varid, Oid typid, int32 typmod, Oid collid,
+ char *attrname, int location)
+{
[...]
+ /*
+ * There are two ways to access session variables - direct, used by simple
+ * plpgsql expressions, where it is not necessary to emulate stability.
+ * And Buffered access, which is used everywhere else. We should ensure
+ * stable values, and because session variables are global, then we should
+ * work with copied values instead of directly accessing variables. For
+ * direct access, the varid is best. For buffered access, we need
+ * to assign an index to the buffer - later, when we know what variables are
+ * used. Now, we just remember, so we use session variables.
I don't understand the last part, starting with "For buffered access, we
need...". Also, the beginning of the comment seems like something more general
and may be moved somewhere, maybe at the beginning of sessionvariable.c?
+ * stmt->query is SelectStmt node. An tranformation of
+ * this node doesn't support SetToDefault node. Instead injecting
+ * of transformSelectStmt or parse state, we can directly
+ * transform target list here if holds SetToDefault node.
+ */
+ if (stmt->set_default)
I don't understand this comment. Especially since the next
transformTargetList() will emit SetToDefault node that will be handled later in
that function and then in RewriteQuery.
+ /*
+ * rewrite SetToDefaults needs varid in Query structure
+ */
+ query->resultVariable = varid;
I also don't understand that comment. Is is always set just in case there's a
SetToDefault, or something else?
+ /* translate paramvarid to session variable name */
+ if (param->paramkind == PARAM_VARIABLE)
+ {
+ appendStringInfo(context->buf, "%s",
+ generate_session_variable_name(param->paramvarid));
+ return;
+ }
A bit more work seems to be needed for deparsing session variables:
# create variable myvar text;
CREATE VARIABLE
# create view myview as select myvar;
CREATE VIEW
# \d+ myview
View "public.myview"
Column | Type | Collation | Nullable | Default | Storage | Description
--------+------+-----------+----------+---------+----------+-------------
myvar | text | | | | extended |
View definition:
SELECT myvar AS myvar;
There shouldn't be an explicit alias I think.
>From 6062665352badd96619fc4849e028bc7e2da9645 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Tue, 1 Mar 2022 21:31:22 +0800
Subject: [PATCH] Suggestion for comment improvements.
---
src/backend/commands/sessionvariable.c | 124 +++++++++++++----------
src/backend/executor/execExpr.c | 12 +--
src/backend/executor/svariableReceiver.c | 11 +-
src/backend/optimizer/plan/setrefs.c | 5 +-
src/backend/optimizer/util/clauses.c | 27 +++--
src/backend/parser/analyze.c | 11 +-
src/backend/parser/gram.y | 10 +-
src/backend/parser/parse_expr.c | 27 ++---
src/backend/utils/cache/lsyscache.c | 2 +-
src/include/catalog/pg_variable.h | 2 +-
10 files changed, 122 insertions(+), 109 deletions(-)
diff --git a/src/backend/commands/sessionvariable.c
b/src/backend/commands/sessionvariable.c
index e94959e5eb..e07ca597bd 100644
--- a/src/backend/commands/sessionvariable.c
+++ b/src/backend/commands/sessionvariable.c
@@ -72,12 +72,13 @@
* the action list.
*
* We want to support the possibility of resetting a session
- * variable at the end of transaction. This ensures the initial
- * state of session variables at the begin of each transaction.
- * The reset is implemented as a removal of the session variable
- * from sessionvars hash table. This enforce full initialization
- * in the next usage. Careful though, this is not same as
- * dropping the session variable.
+ * variable at the end of transaction, with the ON TRANSACTION END
+ * RESET option. This ensures the initial state of session
+ * variables at the begin of each transaction. The reset is
+ * implemented as a removal of the session variable from
+ * sessionvars hash table. This enforce full initialization in
+ * the next usage. Careful though, this is not same as dropping
+ * the session variable.
*
* Another functionality is dropping temporary session variable
* with the option ON COMMIT DROP.
@@ -96,17 +97,15 @@ typedef struct SVariableXActActionItem
SVariableXActAction action; /* reset or drop */
/*
- * If this entry was created during the current transaction,
- * creating_subid is the ID of the creating subxact. If deleted during
- * the current transaction, deleting_subid is the ID of the deleting
- * subxact. if no deletion request is pending, deleting_subid is zero.
- * deletion request is pending, deleting_subid is zero.
+ * creating_subid is the ID of the creating subxact. If the action was
+ * unregistered during the current transaction, deleting_subid is the
ID of
+ * the deleting subxact, otherwise InvalidSubTransactionId.
*/
SubTransactionId creating_subid;
SubTransactionId deleting_subid;
} SVariableXActActionItem;
-/* Both lists holds field of SVariableXActActionItem type */
+/* Both lists hold fields of SVariableXActActionItem type */
static List *xact_drop_actions = NIL;
static List *xact_reset_actions = NIL;
@@ -123,7 +122,7 @@ typedef struct SVariableData
bool is_rowtype; /* true when variable is
composite */
bool is_not_null; /* don't allow null values */
bool is_immutable; /* true when variable is immutable */
- bool has_defexpr; /* true when there are default value */
+ bool has_defexpr; /* true when variable has a default
value */
bool is_valid; /* true when variable was
successfuly
* initialized
*/
@@ -142,8 +141,8 @@ static void register_session_variable_xact_action(Oid
varid, SVariableXActAction
static void unregister_session_variable_xact_action(Oid varid,
SVariableXActAction action);
/*
- * Releases stored data from session variable. The hash entry
- * stay without change.
+ * Releases stored data from session variable, but preserve the hash entry
+ * in sessionvars.
*/
static void
free_session_variable_value(SVariable svar)
@@ -201,9 +200,11 @@ free_session_variable_varid(Oid varid)
}
/*
- * Assign sinval mark to session variable. This mark probably
- * signalized, so the session variable was dropped. But this
- * should be rechecked later against system catalog.
+ * Callback function for session variable invalidation.
+ *
+ * Register SVAR_RECHECK actions on appropriate currently cached
+ * session variables. Those will be processed later, rechecking against the
+ * catalog to detect dropped session variable.
*/
static void
pg_variable_cache_callback(Datum arg, int cacheid, uint32 hashvalue)
@@ -346,7 +347,7 @@ set_session_variable(SVariable svar, Datum value,
/*
* Initialize svar from var
- * svar - SVariable - holds data
+ * svar - SVariable - holds value
* var - Variable - holds metadata
*/
static void
@@ -382,11 +383,13 @@ init_session_variable(SVariable svar, Variable *var)
}
/*
- * Try to search value in hash table. If it doesn't
+ * Search the given session variable in the hash table. If it doesn't
* exist, then insert it (and calculate defexpr if it exists).
*
+ * Caller is responsible for doing permission checks.
+ *
* As side efect this function acquires AccessShareLock on
- * related session variable until commit.
+ * related session variable until the end of the transaction.
*/
static SVariable
prepare_variable_for_reading(Oid varid)
@@ -400,7 +403,7 @@ prepare_variable_for_reading(Oid varid)
if (!sessionvars)
create_sessionvars_hashtable();
- /* Protect used session variable against drop until commit */
+ /* Protect used session variable against drop until transaction end */
LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
svar = (SVariable) hash_search(sessionvars, &varid,
@@ -460,11 +463,13 @@ prepare_variable_for_reading(Oid varid)
}
/*
- * Write value to variable. We expect secured access in this moment.
+ * Store the given value in an SVariable, and cache it if not already present.
+ *
+ * Caller is responsible for doing permission checks.
* We try not to break the previous value, if something is wrong.
*
* As side efect this function acquires AccessShareLock on
- * related session variable until commit.
+ * related session variable until the end of the transaction.
*/
void
SetSessionVariable(Oid varid, Datum value, bool isNull, Oid typid)
@@ -472,7 +477,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid
typid)
SVariable svar;
bool found;
- /* Protect used session variable against drop until commit */
+ /* Protect used session variable against drop until transaction end */
LockDatabaseObject(VariableRelationId, varid, 0, AccessShareLock);
if (!sessionvars)
@@ -486,7 +491,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid
typid)
{
Variable var;
- /* don't need defexpr and acl here */
+ /* We don't need defexpr here */
initVariable(&var, varid, true);
init_session_variable(svar, &var);
}
@@ -495,8 +500,7 @@ SetSessionVariable(Oid varid, Datum value, bool isNull, Oid
typid)
}
/*
- * Write value to variable with security check.
- * We try not to break the previous value, if something is wrong.
+ * Wrapper around SetSessionVariable after checking for correct permission.
*/
void
SetSessionVariableWithSecurityCheck(Oid varid, Datum value, bool isNull, Oid
typid)
@@ -515,6 +519,7 @@ SetSessionVariableWithSecurityCheck(Oid varid, Datum value,
bool isNull, Oid typ
/*
* Returns a copy of value of the session variable specified by varid
+ * Caller is responsible for doing permission checks.
*/
Datum
CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
@@ -536,6 +541,7 @@ CopySessionVariable(Oid varid, bool *isNull, Oid *typid)
/*
* Returns the value of the session variable specified by varid. Check correct
* result type. Optionally the result can be copied.
+ * Caller is responsible for doing permission checks.
*/
Datum
GetSessionVariable(Oid varid, bool *isNull, Oid expected_typid, bool copy)
@@ -696,28 +702,28 @@ RemoveSessionVariable(Oid varid)
table_close(rel, RowExclusiveLock);
/*
- * we removed entry from sys catalog already, we should not to do
- * again at xact time,
+ * We removed entry from catalog already, we should not do it
+ * again at end of xact time.
*/
unregister_session_variable_xact_action(varid, SVAR_ON_COMMIT_DROP);
/*
- * and if this transaction or subtransaction will be committed,
- * we want to enforce variable cleaning. (we don't need to wait for
+ * And we want to enforce variable clearning when this transaction or
+ * subtransaction will be committed (we don't need to wait for
* sinval message). The cleaning action for one session variable
- * can be repeated in the action list, and it doesn't do any problem
- * (so we don't need to ensure uniqueness). We need separate action
- * than RESET, because RESET is executed on any transaction end,
- * but we want to execute cleaning only when thecurrent transaction
+ * can be repeated in the action list without causing any problem,
+ * so we don't need to ensure uniqueness. We need a different action
+ * from RESET, because RESET is executed on any transaction end,
+ * but we want to execute cleaning only when the current transaction
* will be committed.
*/
register_session_variable_xact_action(varid, SVAR_ON_COMMIT_RESET);
}
/*
- * Fast drop complete content of all session variables hash table.
+ * Fast drop of the complete content of all session variables hash table.
* This is code for DISCARD VARIABLES command. This command
- * cannot to run inside transaction, so we don't need to handle
+ * cannot be run inside transaction, so we don't need to handle
* end of transaction actions.
*/
void
@@ -735,7 +741,7 @@ ResetSessionVariables(void)
MemoryContextReset(SVariableMemoryContext);
/*
- * There are not any session variables, so trim
+ * There are not any session variables left, so simply trim
* both xact action lists.
*/
list_free_deep(xact_drop_actions);
@@ -825,11 +831,11 @@ ExecuteLetStmt(ParseState *pstate,
}
/*
- * Implementation of drop or reset actions executed on session variables
- * at transaction end time. We want to drop temporary session variables
- * with clause ON COMMIT DROP, or we want to reset values of session variables
- * with clause ON TRANSACTION END RESET or we want to clean (reset) memory
- * allocated by values of dropped session variables.
+ * Registration of actions to be executed on session variables at transaction
+ * end time. We want to drop temporary session variables with clause ON COMMIT
+ * DROP, or we want to reset values of session variables with clause ON
+ * TRANSACTION END RESET or we want to clean (reset) local memory allocated by
+ * values of dropped session variables.
*/
/*
@@ -862,9 +868,10 @@ register_session_variable_xact_action(Oid varid,
}
/*
- * Remove variable from action list. In this moment,
- * the action is just marked as deleted by setting
- * deleting_subid.
+ * Unregister an action on a given session variable from action list. In this
+ * moment, the action is just marked as deleted by setting deleting_subid. The
+ * calling even might be rollbacked, in which case we should not lose this
+ * action.
*/
static void
unregister_session_variable_xact_action(Oid varid,
@@ -898,7 +905,7 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
SVariableXActActionItem *xact_ai =
(SVariableXActActionItem *) lfirst(l);
- /* Iterate only over non dropped entries */
+ /* Iterate only over entries that are still pending */
if (xact_ai->deleting_subid == InvalidSubTransactionId)
{
Assert(xact_ai->action == SVAR_ON_COMMIT_DROP);
@@ -929,6 +936,10 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
}
}
+ /*
+ * Any drop action left is an entry that was unregistered and not
+ * rollbacked, so we can simply remove them.
+ */
list_free_deep(xact_drop_actions);
xact_drop_actions = NIL;
@@ -940,10 +951,11 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
if (xact_ai->action == SVAR_RECHECK)
{
/*
- * we can do recheck only when transactionn is commited
- * and we can safely touch system catalogue. When
transaction
- * is ROLLBACKed, then we should to postpone check to
next
- * transaction.
+ * We can do the recheck only when the transaction is
commited as
+ * we need to access system catalog. When transaction is
+ * ROLLBACKed, then we have to postpone the check to
next
+ * transaction. We therefore don't check for a matching
+ * creating_subid here.
*/
if (isCommit)
{
@@ -984,6 +996,10 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
isCommit))
free_session_variable_varid(xact_ai->varid);
+ /*
+ * Any non SVAR_RECHECK action can now be removed. It
was either
+ * explicitly processed, or implicitly due to some
ROLLBACK action.
+ */
xact_reset_actions =
foreach_delete_current(xact_reset_actions, l);
pfree(xact_ai);
}
@@ -994,7 +1010,7 @@ AtPreEOXact_SessionVariable_on_xact_actions(bool isCommit)
* Post-subcommit or post-subabort cleanup of xact action list.
*
* During subabort, we can immediately remove entries created during this
- * subtransaction. During subcommit, just relabel entries marked during
+ * subtransaction. During subcommit, just transfer entries marked during
* this subtransaction as being the parent's responsibility.
*/
void
@@ -1025,7 +1041,7 @@ AtEOSubXact_SessionVariable_on_xact_actions(bool
isCommit, SubTransactionId mySu
}
/*
- * Reset and recheck actions - cleaning memory should be used every time
+ * Reset and recheck actions - cleaning memory should be done every time
* (when the variable with short life cycle was used) and then
* cannot be removed from xact action list.
*/
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 834fb4e606..5efdf00de9 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1007,27 +1007,23 @@ ExecInitExprRec(Expr *node, ExprState *state,
es_num_session_variables = state->parent->state->es_num_session_variables;
}
- /*
- * We should use
session variable buffer, when
- * it is available.
- */
+ /* Use session variable
buffer when available. */
if
(es_session_variables)
{
SessionVariableValue *var;
- /* check
params, unexpected */
+ /* Parameter
sanity checks. */
if
(param->paramid >= es_num_session_variables)
elog(ERROR, "paramid of PARAM_VARIABLE param is out of range");
var =
&es_session_variables[param->paramid];
- /* unexpected */
if (var->typid
!= param->paramtype)
elog(ERROR, "type of buffered value is different than PARAM_VARIABLE type");
/*
* In this
case, the parameter is like a
- * constant
+ * constant.
*/
scratch.opcode
= EEOP_CONST;
scratch.d.constval.value = var->value;
@@ -1037,7 +1033,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
else
{
/*
- * When we have
no full PlanState (plpgsql
+ * When we
don't have a full PlanState (plpgsql
* simple expr
evaluation), then we should
* use direct
access.
*/
diff --git a/src/backend/executor/svariableReceiver.c
b/src/backend/executor/svariableReceiver.c
index d5ce377b0f..2cede4ebae 100644
--- a/src/backend/executor/svariableReceiver.c
+++ b/src/backend/executor/svariableReceiver.c
@@ -99,7 +99,7 @@ svariableReceiveSlot(TupleTableSlot *slot, DestReceiver *self)
static void
svariableShutdownReceiver(DestReceiver *self)
{
- /* Do nothing */
+ /* Nothing to do. */
}
/*
@@ -125,13 +125,16 @@ CreateVariableDestReceiver(void)
self->pub.rDestroy = svariableDestroyReceiver;
self->pub.mydest = DestVariable;
- /* private fields will be set by SetVariableDestReceiverParams */
-
+ /*
+ * Private fields will be set by SetVariableDestReceiverParams and
+ * svariableStartupReceiver.
+ */
return (DestReceiver *) self;
}
/*
- * Set parameters for a VariableDestReceiver
+ * Set parameters for a VariableDestReceiver.
+ * Should be called right after creating the DestReceiver.
*/
void
SetVariableDestReceiverParams(DestReceiver *self, Oid varid)
diff --git a/src/backend/optimizer/plan/setrefs.c
b/src/backend/optimizer/plan/setrefs.c
index 7486d975c7..56edecad51 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1749,8 +1749,9 @@ copyVar(Var *var)
* This is code that is common to all variants of expression-fixing.
* We must look up operator opcode info for OpExpr and related nodes,
* add OIDs from regclass Const nodes into root->glob->relationOids, and
- * add PlanInvalItems for user-defined functions into root->glob->invalItems.
- * We also fill in column index lists for GROUPING() expressions.
+ * add PlanInvalItems for user-defined functions and session variables into
+ * root->glob->invalItems. We also fill in column index lists for GROUPING()
+ * expressions.
*
* We assume it's okay to update opcode info in-place. So this could possibly
* scribble on the planner's input data structures, but it's OK.
diff --git a/src/backend/optimizer/util/clauses.c
b/src/backend/optimizer/util/clauses.c
index bd0470bffb..6e29cee23d 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -811,10 +811,10 @@ max_parallel_hazard_walker(Node *node,
max_parallel_hazard_context *context)
/*
* We can't pass Params to workers at the moment either, so they are
also
- * parallel-restricted, unless they are PARAM_EXTERN Params or are
- * PARAM_EXEC Params listed in safe_param_ids, meaning they could be
- * either generated within workers or can be computed by the leader and
- * then their value can be passed to workers.
+ * parallel-restricted, unless they are PARAM_EXTERN or PARAM_VARIABLE
+ * Params or are PARAM_EXEC Params listed in safe_param_ids, meaning
they
+ * could be either generated within workers or can be computed by the
+ * leader and then their value can be passed to workers.
*/
else if (IsA(node, Param))
{
@@ -4764,20 +4764,19 @@ substitute_actual_parameters_mutator(Node *node,
return NULL;
/*
- * The expression of SQL function can contain params of two separated
- * by different paramkind field. The nodes with paramkind PARAM_EXTERN
- * are related to function's arguments (and should be replaced in this
- * step), because this is mechanism how we apply the function's
arguments
- * for an expression.
+ * SQL functions can contain two different kind of params. The nodes
with
+ * paramkind PARAM_EXTERN are related to function's arguments (and
should
+ * be replaced in this step), because this is how we apply the
function's
+ * arguments for an expression.
*
- * The nodes with paramkind PARAM_VARIABLE are related to an usage of
+ * The nodes with paramkind PARAM_VARIABLE are related to usage of
* session variables. The values of session variables are not passed
* to expression by expression arguments, so it should not be replaced
- * here by function's arguments. Although we can substitute params
+ * here by function's arguments. Although we could substitute params
* related to immutable session variables with default expression by
- * this default expression, it is safer don't do it. We don't need to
- * run security checks here. There can be some performance loss, but
- * an access to session variable is fast (and the result of default
+ * this default expression, it is safer to not do it. This way we don't
+ * have to run security checks here. There can be some performance loss,
+ * but an access to session variable is fast (and the result of default
* expression is immediately materialized and can be reused).
*/
if (IsA(node, Param))
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 0249106e63..880c3736aa 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -1686,10 +1686,8 @@ transformLetStmt(ParseState *pstate, LetStmt * stmt)
pstate->p_expr_kind = EXPR_KIND_LET_TARGET;
/*
- * stmt->query is SelectStmt node. An tranformation of
- * this node doesn't support SetToDefault node. Instead injecting
- * of transformSelectStmt or parse state, we can directly
- * transform target list here if holds SetToDefault node.
+ * stmt->query is a SelectStmt node. We don't tranform this node to a
+ * SetToDefault node. Instead we directly transform the target list
here.
*/
if (stmt->set_default)
{
@@ -1854,9 +1852,8 @@ transformLetStmt(ParseState *pstate, LetStmt * stmt)
stmt->query = (Node *) query;
/*
- * When statement is executed as an expression as PLpgSQL LET
- * statement, then we should return query. We should not
- * use a utility wrapper node.
+ * When statement is executed as a PlpgSQL LET statement, then we should
+ * return the query and not use a utilityStmt wrapper node.
*/
if (stmt->plpgsql_mode)
return query;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index bf103ab1ed..a19c56fadb 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4816,11 +4816,11 @@ OptSessionVarDefExpr: DEFAULT b_expr
{ $$ = $2; }
/*
* Temporary session variables can be dropped on successful
- * transaction end like tables. We can force only reset on
- * transaction end. Because the session variables are not
- * transactional, we have calculate with ROLLBACK too.
- * The clause ON TRANSACTION END is more illustrative
- * synonymum to ON COMMIT ROLLBACK RESET.
+ * transaction end like tables. RESET can only be forced on
+ * transaction end. Since the session variables are not
+ * transactional, we have to handle ROLLBACK too.
+ * The clause ON TRANSACTION END is clearer than some
+ * ON COMMIT ROLLBACK RESET clause.
*/
OnEOXActionOption: ON COMMIT DROP { $$ =
VARIABLE_EOX_DROP; }
| ON TRANSACTION END_P RESET { $$ =
VARIABLE_EOX_RESET; }
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 060d74814b..04dbb2bdcf 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -797,10 +797,10 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
parser_errposition(pstate,
cref->location)));
}
- /* Protect session variable by AccessShareLock when it is not shadowed
*/
+ /* Protect session variable by AccessShareLock when it is not shadowed.
*/
lockit = (node == NULL);
- /* The col's reference can be reference to session variable too. */
+ /* The col's reference can be a reference to session variable too. */
varid = IdentifyVariable(cref->fields, &attrname, lockit, ¬_unique);
/*
@@ -823,12 +823,11 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
if (node != NULL)
{
/*
- * In this case we can raise warning (when it is
required).
- * These warnings can be reduced. We should not to raise
- * warning for contexts where usage of session variables
- * has not sense. We should not to raise warning when we
- * can detect so variable has not wanted field (and then
- * there is not possible identifier's collision).
+ * In this case we can raise a warning, but only when
required.
+ * We should not raise warnings for contexts where
usage of session
+ * variables has not sense, or when we can detect that
variable
+ * doesn't have the wanted field (and then there is no
possible
+ * identifier's collision).
*/
if (session_variables_ambiguity_warning &&
pstate->p_expr_kind == EXPR_KIND_SELECT_TARGET)
@@ -843,15 +842,16 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
/*
* Some cases with ambiguous references can be
solved without
- * raising an warning. When there is an
collision between column
+ * raising a warning. When there is a collision
between column
* name (or label) and some session variable
name, and when we
* know attribute name, then we can ignore the
collision when:
*
* a) variable is of scalar type (then
indirection cannot be
* applied on this session variable.
*
- * b) when related variable has no field of
attrname, then
- * indirection cannot be applied on this
session variable.
+ * b) when related variable has no field with
the given
+ * attrname, then indirection cannot be
applied on this
+ * session variable.
*/
if (attrname)
{
@@ -885,7 +885,8 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
}
/*
- * Raise warning when session variable
reference is still valid.
+ * Raise warning when session variable
reference is still
+ * visible.
*/
if (OidIsValid(varid))
ereport(WARNING,
@@ -896,7 +897,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
parser_errposition(pstate, cref->location)));
}
- /* Reference to session variable is shadowed by any
other always. */
+ /* Session variables are always shadowed by any other
node. */
varid = InvalidOid;
}
else
diff --git a/src/backend/utils/cache/lsyscache.c
b/src/backend/utils/cache/lsyscache.c
index e5c3ae697b..46eded5b77 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3587,7 +3587,7 @@ get_varname_varid(const char *varname, Oid varnamespace)
/*
* get_session_variable_name
- * Returns the name of a given session variable.
+ * Returns a palloc'd copy of the name of a given session variable.
*/
char *
get_session_variable_name(Oid varid)
diff --git a/src/include/catalog/pg_variable.h
b/src/include/catalog/pg_variable.h
index 2754b48dde..3dd4455921 100644
--- a/src/include/catalog/pg_variable.h
+++ b/src/include/catalog/pg_variable.h
@@ -55,7 +55,7 @@ typedef enum VariableEOXAction
{
VARIABLE_EOX_NOOP = 'n', /* NOOP */
VARIABLE_EOX_DROP = 'd', /* ON COMMIT DROP */
- VARIABLE_EOX_RESET = 'r', /* ON COMMIT RESET */
+ VARIABLE_EOX_RESET = 'r', /* ON TRANSACTION END RESET */
} VariableEOXAction;
/* ----------------
--
2.33.1
