0002-review-comments-20221105.patch

From 86b3943379d274a93bd177f9ce22831e2a3e3cfb Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas.vondra@postgresql.org>
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