On Fri, Nov 12, 2021 at 02:57:57PM -0300, Alvaro Herrera wrote:
> Here's a new version. Many of the old complaints have been fixed;
I should've replied to this newer message.
I also read through the code a bit and have some more language fixes, mostly.
I ran sqlsmith for a few hours with no apparent issue - good.
It seems like there's an opened question about how RULES should be handled?
--
Justin
>From 119c6c7342ff9410424e94d60dc11b3a5e9d98c9 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 13 Nov 2021 15:47:12 -0600
Subject: [PATCH] f!merge code comments
---
src/backend/commands/trigger.c | 8 +++----
src/backend/executor/execMerge.c | 24 +++++++++----------
src/backend/optimizer/plan/planner.c | 2 +-
src/backend/optimizer/plan/setrefs.c | 2 +-
src/backend/parser/parse_merge.c | 36 +++++++++++++++-------------
src/backend/rewrite/rowsecurity.c | 2 +-
src/bin/psql/tab-complete.c | 2 +-
7 files changed, 39 insertions(+), 37 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index ea5c276cca..e708e4d185 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -3694,9 +3694,9 @@ struct AfterTriggersTableData
AfterTriggerEventList after_trig_events; /* if so, saved list pointer */
/*
- * We maintain separate transaction tables for UPDATE/INSERT/DELETE since
+ * We maintain separate transition tables for UPDATE/INSERT/DELETE since
* MERGE can run all three actions in a single statement. Note that UPDATE
- * needs both old and new transition tables whereas INSERT needs only new
+ * needs both old and new transition tables whereas INSERT needs only new,
* and DELETE needs only old.
*/
@@ -5650,8 +5650,8 @@ AfterTriggerGetTransitionTable(int event,
bool insert_new_table = transition_capture->tcs_insert_new_table;
/*
- * For INSERT events NEW should be non-NULL, for DELETE events OLD should
- * be non-NULL, whereas for UPDATE events normally both OLD and NEW are
+ * For INSERT events, NEW should be non-NULL, for DELETE events, OLD should
+ * be non-NULL, whereas for UPDATE events, normally both OLD and NEW are
* non-NULL. But for UPDATE events fired for capturing transition tuples
* during UPDATE partition-key row movement, OLD is NULL when the event is
* for a row being inserted, whereas NEW is NULL when the event is for a
diff --git a/src/backend/executor/execMerge.c b/src/backend/executor/execMerge.c
index a9bd103f41..aabe10d14c 100644
--- a/src/backend/executor/execMerge.c
+++ b/src/backend/executor/execMerge.c
@@ -66,9 +66,9 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
/*
* We run a JOIN between the target relation and the source relation to
- * find a set of candidate source rows that has matching row in the target
- * table and a set of candidate source rows that does not have matching
- * row in the target table. If the join returns us a tuple with target
+ * find a set of candidate source rows that have a matching row in the target
+ * table and a set of candidate source rows that does not have a matching
+ * row in the target table. If the join returns a tuple with the target
* relation's tid set, that implies that the join found a matching row for
* the given source tuple. This case triggers the WHEN MATCHED clause of
* the MERGE. Whereas a NULL in the target relation's ctid column
@@ -122,7 +122,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
* XXX Hmmm, what if the updated tuple would now match one that was
* considered NOT MATCHED so far?
*
- * A concurrent delete, changes a WHEN MATCHED case to WHEN NOT MATCHED.
+ * A concurrent delete changes a WHEN MATCHED case to WHEN NOT MATCHED.
*
* ExecMergeMatched takes care of following the update chain and
* re-finding the qualifying WHEN MATCHED action, as long as the updated
@@ -140,7 +140,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
/*
* Either we were dealing with a NOT MATCHED tuple or
* ExecMergeNotMatched() returned "false", indicating the previously
- * MATCHED tuple is no longer a matching tuple.
+ * MATCHED tuple no longer matches.
*/
if (!matched)
ExecMergeNotMatched(mtstate, resultRelInfo, estate, slot);
@@ -223,7 +223,7 @@ lmerge_matched:
/*
* Test condition, if any
*
- * In the absence of a condition we perform the action unconditionally
+ * In the absence of any condition, we perform the action unconditionally
* (no need to check separately since ExecQual() will return true if
* there are no conditions to evaluate).
*/
@@ -231,14 +231,14 @@ lmerge_matched:
continue;
/*
- * Check if the existing target tuple meet the USING checks of
+ * Check if the existing target tuple meets the USING checks of
* UPDATE/DELETE RLS policies. If those checks fail, we throw an
* error.
*
* The WITH CHECK quals are applied in ExecUpdate() and hence we need
* not do anything special to handle them.
*
- * NOTE: We must do this after WHEN quals are evaluated so that we
+ * NOTE: We must do this after WHEN quals are evaluated, so that we
* check policies only when they matter.
*/
if (resultRelInfo->ri_WithCheckOptions)
@@ -340,7 +340,7 @@ lmerge_matched:
*
* If the current tuple was concurrently updated, then we
* must run the EvalPlanQual() with the new version of the
- * tuple. If EvalPlanQual() does not return a tuple then
+ * tuple. If EvalPlanQual() does not return a tuple, then
* we switch to the NOT MATCHED list of actions. If it
* does return a tuple and the join qual is still
* satisfied, then we just need to recheck the MATCHED
@@ -496,7 +496,7 @@ ExecMergeNotMatched(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
ListCell *l;
/*
- * For INSERT actions, root relation's merge action is OK since the
+ * For INSERT actions, the root relation's merge action is OK since the
* INSERT's targetlist and the WHEN conditions can only refer to the
* source relation and hence it does not matter which result relation we
* work with.
@@ -508,7 +508,7 @@ ExecMergeNotMatched(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
/*
* Make source tuple available to ExecQual and ExecProject. We don't need
- * the target tuple since the WHEN quals and the targetlist can't refer to
+ * the target tuple, since the WHEN quals and the targetlist can't refer to
* the target columns.
*/
econtext->ecxt_scantuple = NULL;
@@ -523,7 +523,7 @@ ExecMergeNotMatched(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
/*
* Test condition, if any
*
- * In the absence of a condition we perform the action unconditionally
+ * In the absence of a condition, we perform the action unconditionally
* (no need to check separately since ExecQual() will return true if
* there are no conditions to evaluate).
*/
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 159c38ae91..89104fccb6 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1811,7 +1811,7 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
/*
* Copy MergeActions and translate stuff that
- * reference attribute numbers.
+ * references attribute numbers.
*/
foreach(l, parse->mergeActionList)
{
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 476583a85e..f627b832f1 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1035,7 +1035,7 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
* a right join between the target relation and the source
* relation (which could be a plain relation or a subquery).
* The INSERT and UPDATE actions of the MERGE statement
- * requires access to the columns from the source relation. We
+ * require access to the columns from the source relation. We
* arrange things so that the source relation attributes are
* available as INNER_VAR and the target relation attributes
* are available from the scan tuple.
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index e183147ea8..aa28293407 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -47,7 +47,7 @@ static void setNamespaceVisibilityForRTE(List *namespace, RangeTblEntry *rte,
* Process the FROM clause and add items to the query's range table,
* joinlist, and namespace.
*
- * A special targetlist comprising of the columns from the right-subtree of
+ * A special targetlist comprised of the columns from the right-subtree of
* the join is populated and returned. Note that when the JoinExpr is
* setup by transformMergeStmt, the left subtree has the target result
* relation and the right subtree has the source relation.
@@ -86,14 +86,14 @@ transformMergeJoinClause(ParseState *pstate, Node *merge,
* the joining relations, unless the column references are qualified.
* Also, any unqualified column references are resolved to the Join RTE,
* if there is a matching entry in the targetlist. But the way MERGE
- * execution is later setup, we expect all column references to resolve to
+ * execution is later set up, we expect all column references to resolve to
* either the source or the target relation. Hence we must not add the
* Join RTE to the namespace.
*
* The last entry must be for the top-level Join RTE. We don't want to
* resolve any references to the Join RTE. So discard that.
*
- * We also do not want to resolve any references from the leftside of the
+ * We also do not want to resolve any references from the left side of the
* Join since that corresponds to the target relation. References to the
* columns of the target relation must be resolved from the result
* relation and not the one that is used in the join.
@@ -252,12 +252,13 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
errmsg("unreachable WHEN clause specified after unconditional WHEN clause")));
}
- /*
+ /* ---
* Construct a query of the form SELECT relation.ctid --junk attribute
- * ,relation.tableoid --junk attribute ,source_relation.<somecols>
- * ,relation.<somecols> FROM relation RIGHT JOIN source_relation ON
- * join_condition; -- no WHERE clause - all conditions are applied in
- * executor
+ * ,relation.tableoid --junk attribute
+ * ,source_relation.<somecols>
+ * ,relation.<somecols>
+ * FROM relation RIGHT JOIN source_relation ON join_condition;
+ * -- no WHERE clause - all conditions are applied in executor
*
* stmt->relation is the target relation, given as a RangeVar
* stmt->source_relation is a RangeVar or subquery
@@ -275,17 +276,18 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
* of candidate change rows, so the query must contain all of the columns
* required by each of the targetlist or conditions for each action.
*
- * As top-level statements INSERT, UPDATE and DELETE have a Query, whereas
- * with MERGE the individual actions do not require separate planning,
+ * As top-level statements, INSERT, UPDATE and DELETE have a Query, whereas
+ * for MERGE the individual actions do not require separate planning,
* only different handling in the executor. See nodeModifyTable handling
* of commandType CMD_MERGE.
*
* A sub-query can include the Target, but otherwise the sub-query cannot
* reference the outermost Target table at all.
+ * ---
*/
/*
- * Setup the MERGE target table.
+ * Set up the MERGE target table.
*/
qry->resultRelation = setTargetTable(pstate, stmt->relation,
stmt->relation->inh,
@@ -309,7 +311,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
* These seem like things that could go into Optimizer, but they are
* semantic simplifications rather than optimizations, per se.
*
- * If there are no INSERT actions we won't be using the non-matching
+ * If there are no INSERT actions, we won't be using the non-matching
* candidate rows for anything, so no need for an outer join. We do still
* need an inner join for UPDATE and DELETE actions.
*/
@@ -322,7 +324,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
* We use a special purpose transformation here because the normal
* routines don't quite work right for the MERGE case.
*
- * A special mergeSourceTargetList is setup by transformMergeJoinClause().
+ * A special mergeSourceTargetList is set up by transformMergeJoinClause().
* It refers to all the attributes output by the join.
*/
transformMergeJoinClause(pstate, (Node *) joinexpr,
@@ -343,8 +345,8 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
/*
* XXX MERGE is unsupported in various cases
*/
- if (!(pstate->p_target_relation->rd_rel->relkind == RELKIND_RELATION ||
- pstate->p_target_relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE))
+ if (pstate->p_target_relation->rd_rel->relkind != RELKIND_RELATION &&
+ pstate->p_target_relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("MERGE is not supported for this relation type")));
@@ -361,7 +363,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
errmsg("MERGE is not supported for relations with rules")));
/*
- * We now have a good query shape, so now look at the when conditions and
+ * We now have a good query shape, so now look at the WHEN conditions and
* action targetlists.
*
* Overall, the MERGE Query's targetlist is NIL.
@@ -391,7 +393,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
setNamespaceForMergeWhen(pstate, mergeWhenClause);
/*
- * Transform the when condition.
+ * Transform the WHEN condition.
*
* Note that these quals are NOT added to the join quals; instead they
* are evaluated separately during execution to decide which of the
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 1c9aa22422..b608553acd 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -391,7 +391,7 @@ get_row_security_policies(Query *root, RangeTblEntry *rte, int rt_index,
*
* We don't push the UPDATE/DELETE USING quals to the RTE because we don't
* really want to apply them while scanning the relation since we don't
- * know whether we will be doing a UPDATE or a DELETE at the end. We apply
+ * know whether we will be doing an UPDATE or a DELETE at the end. We apply
* the respective policy once we decide the final action on the target
* tuple.
*
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index a2d4755a6f..650a7fb5d8 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -536,7 +536,7 @@ static const SchemaQuery Query_for_list_of_updatables = {
/* Relations supporting MERGE */
static const SchemaQuery Query_for_list_of_mergetargets = {
/* min_server_version */
- 110000,
+ 150000,
/* catname */
"pg_catalog.pg_class c",
/* selcondition */
--
2.17.0
