From a332b1c279e847ec533ff34f4444fc35303672cd Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Sat, 9 Jan 2021 03:17:56 +0100 Subject: [PATCH 1/2] review --- doc/src/sgml/mvcc.sgml | 20 +++++++++++--------- doc/src/sgml/ref/create_policy.sgml | 6 +++--- doc/src/sgml/ref/merge.sgml | 12 +++++++++--- src/backend/commands/explain.c | 2 +- src/backend/executor/execMerge.c | 5 ++++- src/backend/executor/nodeModifyTable.c | 3 ++- src/backend/optimizer/prep/preptlist.c | 6 ++++++ src/backend/parser/parse_agg.c | 1 + src/backend/replication/walsender.c | 4 ++-- src/backend/rewrite/rewriteHandler.c | 4 ++++ src/backend/utils/adt/varlena.c | 3 +++ src/include/nodes/parsenodes.h | 7 ++++--- 12 files changed, 50 insertions(+), 23 deletions(-) diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index 9ec8474185..02c9f3fdea 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -429,22 +429,24 @@ COMMIT; with both INSERT and UPDATE subcommands looks similar to INSERT with an ON CONFLICT DO UPDATE clause but does not guarantee - that either INSERT and UPDATE will occur. + that either INSERT or UPDATE will occur. - If MERGE attempts an UPDATE or DELETE and the row is concurrently updated + /* XXX This is a very long and hard to understand sentence :-( */ + /* XXX Do we want to mention EvalPlanQual here? There's no explanation what it does in this file, so maybe elaborate what it does or leave that detail for a README? */ + If MERGE attempts an UPDATE or DELETE and the row is concurrently updated but the join condition still passes for the current target and the current - source tuple, then MERGE will behave the same as the UPDATE or DELETE commands + source tuple, then MERGE will behave the same as the UPDATE or DELETE commands and perform its action on the latest version of the row, using standard - EvalPlanQual. MERGE actions can be conditional, so conditions must be + EvalPlanQual. MERGE actions can be conditional, so conditions must be re-evaluated on the latest row, starting from the first action. On the other hand, if the row is concurrently updated or deleted so that - the join condition fails, then MERGE will execute a NOT MATCHED action, if it - exists and the AND WHEN qual evaluates to true. + the join condition fails, then MERGE will execute a NOT MATCHED action, if it + exists and the AND WHEN qual evaluates to true. - If MERGE attempts an INSERT and a unique index is present and a duplicate - row is concurrently inserted then a uniqueness violation is raised. MERGE - does not attempt to avoid the ERROR by attempting an UPDATE. + If MERGE attempts an INSERT and a unique index is present and a duplicate + row is concurrently inserted then a uniqueness violation is raised. MERGE + does not attempt to avoid the ERROR by attempting an UPDATE. diff --git a/doc/src/sgml/ref/create_policy.sgml b/doc/src/sgml/ref/create_policy.sgml index ad20230c58..0a64674f2d 100644 --- a/doc/src/sgml/ref/create_policy.sgml +++ b/doc/src/sgml/ref/create_policy.sgml @@ -97,9 +97,9 @@ CREATE POLICY name ON No separate policy exists for MERGE. Instead policies - defined for SELECT, INSERT, - UPDATE and DELETE are applied - while executing MERGE, depending on the actions that are activated. + defined for SELECT, INSERT, + UPDATE and DELETE are applied + while executing MERGE, depending on the actions that are activated. diff --git a/doc/src/sgml/ref/merge.sgml b/doc/src/sgml/ref/merge.sgml index b2a9f67cfa..c2901b0d58 100644 --- a/doc/src/sgml/ref/merge.sgml +++ b/doc/src/sgml/ref/merge.sgml @@ -98,7 +98,7 @@ DELETE - There is no MERGE privilege. + There is no MERGE privilege. You must have the UPDATE privilege on the column(s) of the target_table_name referred to in the SET clause @@ -217,6 +217,7 @@ DELETE At least one WHEN clause is required. + /* XXX Do we need to repeat the same thing for WHEN MATCHED and WHEN NOT MATCHED? Could this say that it applies to both? */ If the WHEN clause specifies WHEN MATCHED and the candidate change row matches a row in the target_table_name @@ -242,6 +243,7 @@ DELETE clause will be activated and the corresponding action will occur for that row. The expression may not contain functions that possibly performs writes to the database. + /* XXX Does it mean that it has to be marked as STABLE, or that a write crashes the DB? */ A condition on a WHEN MATCHED clause can refer to columns @@ -379,6 +381,7 @@ DELETE An expression to assign to the column. The expression can use the old values of this and other columns in the table. + /* XXX Which table? Source or target, or both? What if it's NOT MATCHED? */ @@ -492,6 +495,7 @@ MERGE total-count In summary, statement triggers for an event type (say, INSERT) will be fired whenever we specify an action of that kind. Row-level triggers will fire only for the one event type activated. + /* XXX What does "activated" mean? Maybe "executed" would be better? */ So a MERGE might fire statement triggers for both UPDATE and INSERT, even though only UPDATE row triggers were fired. @@ -504,6 +508,8 @@ MERGE total-count rows will be used to modify the target row, later attempts to modify will cause an error. This can also occur if row triggers make changes to the target table which are then subsequently modified by MERGE. + /* XXX Not sure the preceding sentence makes sense. What does the 'which' refer to? Row triggers, changes, or what? */ + /* XXX It seems the rule is that INSERT actions are while INSERT statements (in SQL sense) are . But this mixes that up? */ If the repeated action is an INSERT this will cause a uniqueness violation while a repeated UPDATE or DELETE will cause a cardinality violation; the latter behavior @@ -554,7 +560,7 @@ MERGE total-count Examples - Perform maintenance on CustomerAccounts based upon new Transactions. + Perform maintenance on CustomerAccounts based upon new Transactions. MERGE CustomerAccount CA @@ -599,7 +605,7 @@ WHEN MATCHED THEN DELETE; - The wine_stock_changes table might be, for example, a temporary table + The wine_stock_changes table might be, for example, a temporary table recently loaded into the database. diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index fbaf50c258..f914a00e9f 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -3690,7 +3690,7 @@ show_modifytable_info(ModifyTableState *mtstate, List *ancestors, break; case CMD_MERGE: operation = "Merge"; - foperation = "Foreign Merge"; + foperation = "Foreign Merge"; /* XXX Doesn't the commit message say foreign tables are not yet supported? */ break; default: operation = "???"; diff --git a/src/backend/executor/execMerge.c b/src/backend/executor/execMerge.c index 0e245e1361..a7492a6c4b 100644 --- a/src/backend/executor/execMerge.c +++ b/src/backend/executor/execMerge.c @@ -105,7 +105,7 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo, * A concurrent update can: * * 1. modify the target tuple so that it no longer satisfies the - * additional quals attached to the current WHEN MATCHED action OR + * additional quals attached to the current WHEN MATCHED action * * In this case, we are still dealing with a WHEN MATCHED case, but we * should recheck the list of WHEN MATCHED actions and choose the first @@ -118,6 +118,9 @@ ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo, * tuple, so we now instead find a qualifying WHEN NOT MATCHED action to * execute. * + * 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. * * ExecMergeMatched takes care of following the update chain and diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index c0a97eba91..9c14709e47 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2124,7 +2124,6 @@ ExecModifyTable(PlanState *pstate) { /* advance to next subplan if any */ node->mt_whichplan++; - if (node->mt_whichplan < node->mt_nplans) { resultRelInfo++; @@ -2169,6 +2168,8 @@ ExecModifyTable(PlanState *pstate) EvalPlanQualSetSlot(&node->mt_epqstate, planSlot); slot = planSlot; + /* XXX Wouldn't it be "nicer" to handle MERGE in the switch, together + * with the other MT operations? This seems a bit out of place. */ if (operation == CMD_MERGE) { ExecMerge(node, resultRelInfo, estate, slot, junkfilter); diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 8848e9a03f..b2543f6814 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -117,6 +117,10 @@ preprocess_targetlist(PlannerInfo *root) tlist = expand_targetlist(tlist, command_type, result_relation, target_relation); + /* + * For MERGE we need to handle the target list for the target relation, + * and also target list for each action (only INSERT/UPDATE matter). + */ if (command_type == CMD_MERGE) { ListCell *l; @@ -343,6 +347,8 @@ expand_targetlist(List *tlist, int command_type, * generate a NULL for dropped columns (we want to drop any old * values). * + * XXX Should this explain why MERGE has the same logic as UPDATE? + * * When generating a NULL constant for a dropped column, we label * it INT4 (any other guaranteed-to-exist datatype would do as * well). We can't label it with the dropped column's datatype diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c index f7c152beb4..5d49c8c37e 100644 --- a/src/backend/parser/parse_agg.c +++ b/src/backend/parser/parse_agg.c @@ -436,6 +436,7 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr) break; case EXPR_KIND_MERGE_WHEN_AND: if (isAgg) + /* XXX "WHEN AND" seems rather strange. ... */ err = _("aggregate functions are not allowed in WHEN AND conditions"); else err = _("grouping operations are not allowed in WHEN AND conditions"); diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index d59f4fde95..d50543b1ba 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1198,6 +1198,7 @@ StartLogicalReplication(StartReplicationCmd *cmd) /* Also update the sent position status in shared memory */ SpinLockAcquire(&MyWalSnd->mutex); + /* XXX Huh? Why does MERGE patch change walsender? */ MyWalSnd->sentPtr = MyReplicationSlot->data.confirmed_flush; SpinLockRelease(&MyWalSnd->mutex); @@ -2930,8 +2931,7 @@ WalSndDone(WalSndSendDataCallback send_data) replicatedPtr = XLogRecPtrIsInvalid(MyWalSnd->flush) ? MyWalSnd->write : MyWalSnd->flush; - if (WalSndCaughtUp && - sentPtr == replicatedPtr && + if (WalSndCaughtUp && sentPtr == replicatedPtr && !pq_is_send_pending()) { QueryCompletion qc; diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index e2706c181c..80ae946d17 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1774,6 +1774,7 @@ fill_extraUpdatedCols(RangeTblEntry *target_rte, Relation target_relation) } } + /* * matchLocks - * match the list of locks and returns the matching rules @@ -3946,6 +3947,9 @@ RewriteQuery(Query *parsetree, List *rewrite_events) /* * XXX MERGE doesn't support write rules because they would violate * the SQL Standard spec and would be unclear how they should work. + * + * XXX So does't support means 'ignores'? Should that either fail + * or at least print some warning? */ if (event == CMD_MERGE) product_queries = NIL; diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 0281351dcf..7a768a7b5b 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2312,6 +2312,7 @@ varstrfastcmp_locale(char *a1p, int len1, char *a2p, int len2, SortSupport ssup) int result; bool arg1_match; + /* XXX Huh? Why is this in MERGE patch? */ if (len1 < 0 || len2 < 0) ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), @@ -2505,6 +2506,7 @@ varstr_abbrev_convert(Datum original, SortSupport ssup) memset(pres, 0, sizeof(Datum)); len = VARSIZE_ANY_EXHDR(authoritative); + /* XXX Huh? Why is this in MERGE patch? */ if (len < 0) ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), @@ -3260,6 +3262,7 @@ bytea_catenate(bytea *t1, bytea *t2) len1 = VARSIZE_ANY_EXHDR(t1); len2 = VARSIZE_ANY_EXHDR(t2); + /* XXX Huh? Why is this in MERGE patch? */ if (len1 < 0 || len2 < 0) ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 3cba38044e..08fea9b8f7 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -172,6 +172,7 @@ typedef struct Query List *withCheckOptions; /* a list of WithCheckOption's (added * during rewrite) */ + /* XXX Why not mergeTragetRelation? */ int mergeTarget_relation; List *mergeSourceTargetList; List *mergeActionList; /* list of actions for MERGE (only) */ @@ -1581,11 +1582,11 @@ typedef struct UpdateStmt typedef struct MergeStmt { NodeTag type; - RangeVar *relation; /* target relation to merge into */ + RangeVar *relation; /* target relation to merge into */ Node *source_relation; /* source relation */ - Node *join_condition; /* join condition between source and target */ + Node *join_condition; /* join condition between source and target */ List *mergeWhenClauses; /* list of MergeWhenClause(es) */ - WithClause *withClause; /* WITH clause */ + WithClause *withClause; /* WITH clause */ } MergeStmt; typedef struct MergeWhenClause -- 2.26.2