From 86b3943379d274a93bd177f9ce22831e2a3e3cfb Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 5 Nov 2022 00:03:34 +0100 Subject: [PATCH 02/15] review comments --- src/backend/access/transam/xact.c | 3 +++ src/backend/catalog/aclchk.c | 2 -- src/backend/commands/Makefile | 2 +- src/backend/commands/tablecmds.c | 3 ++- src/backend/utils/adt/acl.c | 1 + src/backend/utils/fmgr/fmgr.c | 3 +++ src/include/catalog/namespace.h | 2 ++ src/tools/pgindent/typedefs.list | 3 ++- 8 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 6643739e53d..a05f18ab23e 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -2215,6 +2215,8 @@ CommitTransaction(void) smgrDoPendingSyncs(true, is_parallel_worker); /* Let ON COMMIT DROP */ + /* review: everything else is called just AtEOXact_. Also why suffix + * _on_xact_actions? Seems redundant with AtEOXact_ prefix. */ AtPreEOXact_SessionVariable_on_xact_actions(true); /* close large objects before lower-level cleanup */ @@ -2795,6 +2797,7 @@ AbortTransaction(void) AtEOXact_LargeObject(false); /* 'false' means it's abort */ + /* review: same naming issues as for commit */ AtPreEOXact_SessionVariable_on_xact_actions(false); AtAbort_Notify(); AtEOXact_RelationMap(false, is_parallel_worker); diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 11b2788529b..fb7e3a223c7 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -945,7 +945,6 @@ objectsInSchemaToOids(ObjectType objtype, List *nspnames) table_close(rel, AccessShareLock); } break; - default: /* should not happen */ elog(ERROR, "unrecognized GrantStmt.objtype: %d", @@ -6024,7 +6023,6 @@ pg_variable_ownercheck(Oid db_oid, Oid roleid) return has_privs_of_role(roleid, ownerId); } - /* * Check whether specified role has CREATEROLE privilege (or is a superuser) * diff --git a/src/backend/commands/Makefile b/src/backend/commands/Makefile index 642454c745c..1cfaeca51ea 100644 --- a/src/backend/commands/Makefile +++ b/src/backend/commands/Makefile @@ -48,9 +48,9 @@ OBJS = \ proclang.o \ publicationcmds.o \ schemacmds.o \ - session_variable.o \ seclabel.o \ sequence.o \ + session_variable.o \ statscmds.o \ subscriptioncmds.o \ tablecmds.o \ diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 4c484e2135f..c2f2eaf8e8b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -6499,7 +6499,7 @@ find_composite_type_dependencies(Oid typeOid, Relation origRelation, continue; } - /* Don't allow change of type usedby session's variable */ + /* Don't allow change of type used by session's variable */ if (pg_depend->classid == VariableRelationId) { Oid varid = pg_depend->objid; @@ -6538,6 +6538,7 @@ find_composite_type_dependencies(Oid typeOid, Relation origRelation, /* Else, ignore dependees that aren't user columns of relations */ /* (we assume system columns are never of interesting types) */ + /* review: why is this in "else" branch? */ else if (pg_depend->classid != RelationRelationId || pg_depend->objsubid <= 0) continue; diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c index 80a46b0925a..913e0f5fa44 100644 --- a/src/backend/utils/adt/acl.c +++ b/src/backend/utils/adt/acl.c @@ -1612,6 +1612,7 @@ makeaclitem(PG_FUNCTION_ARGS) PG_RETURN_ACLITEM_P(result); } + /* * convert_any_priv_string: recognize privilege strings for has_foo_privilege * diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c index af74fe345ed..3fe55edcc81 100644 --- a/src/backend/utils/fmgr/fmgr.c +++ b/src/backend/utils/fmgr/fmgr.c @@ -1899,6 +1899,9 @@ get_call_expr_arg_stable(Node *expr, int argnum) * Either a true Const or an external Param or variable will have a value * that doesn't change during the execution of the query. In future we * might want to consider other cases too, e.g. now(). + * + * review: Why does this update the comment, when the code did not change + * at all? Is a variable represented as Param, or what? */ if (IsA(arg, Const)) return true; diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h index e6873d369af..ca6f120da79 100644 --- a/src/include/catalog/namespace.h +++ b/src/include/catalog/namespace.h @@ -166,9 +166,11 @@ extern void SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId); extern void ResetTempTableNamespace(void); +/* review: NamesFromList is only used in 0003, so maybe don't add it before that? */ extern List *NamesFromList(List *names); extern Oid LookupVariable(const char *nspname, const char *varname, bool rowtype_only, bool missing_ok); +/* review: IdentifyVariable is only used in 0002, so maybe don't add it before that? */ extern Oid IdentifyVariable(List *names, char **attrname, bool *not_unique); extern OverrideSearchPath *GetOverrideSearchPath(MemoryContext context); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 6778bbb80d1..ebf2e11456c 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -826,6 +826,7 @@ FormData_pg_ts_parser FormData_pg_ts_template FormData_pg_type FormData_pg_user_mapping +FormData_pg_variable Form_pg_aggregate Form_pg_am Form_pg_amop @@ -884,7 +885,7 @@ Form_pg_ts_parser Form_pg_ts_template Form_pg_type Form_pg_user_mapping -FormData_pg_variable +Form_pg_variable FormatNode FreeBlockNumberArray FreeListData -- 2.38.1