Re: support for MERGE

Hi,

On 2022-01-28 17:27:37 -0300, Alvaro Herrera wrote:
> MERGE, v10. I am much more comfortable with this version; I have
> removed a bunch of temporary hacks and cleaned up the interactions with
> table AM and executor, which is something that had been bothering me for
> a while. The complete set of changes can be seen in github,
> https://github.com/alvherre/postgres/commits/merge-15

Any chance you could split this into something more reviewable? The overall
diff stats are: 102 files changed, 8589 insertions(+), 234 deletions(-) thats
pretty hard to really review. Incremental commits don't realy help with that.

> I am not aware of anything of significance in terms of remaining work
> for this project.

One thing from skimming: There's not enough documentation about the approach
imo - it's a complicated enough feature to deserve more than the one paragraph
in src/backend/executor/README.

I'm still [1],[2] uncomfortable with execMerge.c kinda-but-not-really being an
executor node.

> The one thing I'm a bit bothered about is the fact that we expose a lot of
> executor functions previously static. I am now wondering if it would be
> better to move the MERGE executor support functions into nodeModifyTable.c,
> which I think would mean we would not have to expose those function
> prototypes.

I'm worried about the complexity of nodeModifyTuple.c as is. Moving more code
in there does not seem like the right call to me - we should do the opposite
if anything.

A few inline comments below. No real review, just stuff noticed while skimming
to see where this is at.

Greetings,

Andres

[1] https://www.postgresql.org/message-id/20180405200003.gar3j26gsk32gqpe@alap3.anarazel.de
[2] https://www.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup@alap3.anarazel.de

> diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> index 1a9c1ac290..280ac40e63 100644
> --- a/src/backend/commands/trigger.c
> +++ b/src/backend/commands/trigger.c

This stuff seems like one candidate for splitting out.

> + /*
> + * 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,
> + * and DELETE needs only old.
> + */
> +
> + /* "old" transition table for UPDATE, if any */
> + Tuplestorestate *old_upd_tuplestore;
> + /* "new" transition table for UPDATE, if any */
> + Tuplestorestate *new_upd_tuplestore;
> + /* "old" transition table for DELETE, if any */
> + Tuplestorestate *old_del_tuplestore;
> + /* "new" transition table INSERT, if any */
> + Tuplestorestate *new_ins_tuplestore;
> +
> TupleTableSlot *storeslot; /* for converting to tuplestore's format */
> };

Do we need to do something slightly clever about the memory limits for the
tuplestore? Each of them having a separate work_mem makes nodeModifyTable.c
one memory hungry node (the worst of all maybe?).

> +
> +/*
> + * Perform MERGE.
> + */
> +TupleTableSlot *
> +ExecMerge(ModifyTableState *mtstate, ResultRelInfo *resultRelInfo,
> + EState *estate, TupleTableSlot *slot)
> +{
> + ExprContext *econtext = mtstate->ps.ps_ExprContext;
> + ItemPointer tupleid;
> + ItemPointerData tuple_ctid;
> + bool matched = false;
> + Datum datum;
> + bool isNull;
> +
> + /*
> + * Reset per-tuple memory context to free any expression evaluation
> + * storage allocated in the previous cycle.
> + */
> + ResetExprContext(econtext);

Hasn't this already happened in ExecModifyTable()?

> + /*
> + * We run a JOIN between the target relation and the source relation to
> + * find a set of candidate source rows that have a matching row in the
> + * target table and a set of candidate source rows that do not have a
> + * matching row in the target table. If the join returns a tuple with the
> + * target relation's row-ID 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
> + * row-ID column indicates a NOT MATCHED case.
> + */
> + datum = ExecGetJunkAttribute(slot,
> + resultRelInfo->ri_RowIdAttNo,
> + &isNull);

Could this be put somewhere common with the equivalent call in
ExecModifyTable()?

> + * 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
> + *
> + * In this case, we are still dealing with a WHEN MATCHED case.
> + * In this case, we recheck the list of WHEN MATCHED actions from
> + * the start and choose the first one that satisfies the new target
> + * tuple.

Duplicated "in this case"

> + case TM_Invisible:
> +
> + /*
> + * This state should never be reached since the underlying
> + * JOIN runs with a MVCC snapshot and EvalPlanQual runs
> + * with a dirty snapshot. So such a row should have never
> + * been returned for MERGE.
> + */
> + elog(ERROR, "unexpected invisible tuple");
> + break;

Seems like it was half duplicated at some point?

> + case TM_Updated:
> + case TM_Deleted:
> +
> + /*
> + * The target tuple was concurrently updated/deleted by
> + * some other transaction.
> + *
> + * If the current tuple is the last tuple in the update
> + * chain, then we know that the tuple was concurrently
> + * deleted. Just return and let the caller try NOT MATCHED
> + * actions.

Is it really correct to behave that way when using repeatable read /
serializable?

> + /*
> + * Project the tuple. In case of a partitioned table, the
> + * projection was already built to use the root's descriptor,
> + * so we don't need to map the tuple here.
> + */
> + actionInfo.actionState = action;
> + insert_slot = ExecProject(action->mas_proj);
> +
> + (void) ExecInsert(mtstate, rootRelInfo,
> + insert_slot, slot,
> + estate, &actionInfo,
> + mtstate->canSetTag);
> + InstrCountFiltered1(&mtstate->ps, 1);

What happens if somebody concurrently inserts a conflicting row?

> --- a/src/include/executor/instrument.h
> +++ b/src/include/executor/instrument.h
> @@ -82,8 +82,11 @@ typedef struct Instrumentation
> double ntuples; /* total tuples produced */
> double ntuples2; /* secondary node-specific tuple counter */
> double nloops; /* # of run cycles for this node */
> - double nfiltered1; /* # of tuples removed by scanqual or joinqual */
> - double nfiltered2; /* # of tuples removed by "other" quals */
> + double nfiltered1; /* # tuples removed by scanqual or joinqual OR
> + * # tuples inserted by MERGE */
> + double nfiltered2; /* # tuples removed by "other" quals OR #
> + * tuples updated by MERGE */
> + double nfiltered3; /* # tuples deleted by MERGE */

It was a bad idea to have nfiltered1/2. Arriving at nfiltered3, it seems we
really should rename them...