From 6062665352badd96619fc4849e028bc7e2da9645 Mon Sep 17 00:00:00 2001 From: Julien Rouhaud 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