Re: [HACKERS] MERGE SQL Statement for PG11

On Tue, Jan 23, 2018 at 5:51 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Not yet working
>> * Partitioning
>> * RLS
>>
>> Based on this successful progress I imagine I'll be looking to commit
>> this by the end of the CF, allowing us 2 further months to bugfix.
>
> This is complete and pretty clean now. 1200 lines of code, plus docs and tests.

That timeline seems aggressive to me. Also, the patch appears to have
bitrot. Please rebase, and post a new version.

Some feedback based on a short read-through of your v11:

* What's the postgres_fdw status?

* Relatedly, when/where does applying the junkfilter happen, if not here?:

> @@ -1767,9 +1833,9 @@ ExecModifyTable(PlanState *pstate)
> }
>
> /*
> - * apply the junkfilter if needed.
> + * apply the junkfilter if needed - we do this later for CMD_MERGE
> */
> - if (operation != CMD_DELETE)
> + if (operation == CMD_UPDATE || operation == CMD_INSERT)
> slot = ExecFilterJunk(junkfilter, slot);

* Isn't "consider INSERT ... ON CONFLICT DO UPDATE" obsolete in these
doc changes?:

> -F312 MERGE statement NO consider INSERT ... ON CONFLICT DO UPDATE
> -F313 Enhanced MERGE statement NO
> -F314 MERGE statement with DELETE branch NO
> +F312 MERGE statement YES consider INSERT ... ON CONFLICT DO UPDATE
> +F313 Enhanced MERGE statement YES
> +F314 MERGE statement with DELETE branch YES

* What's the deal with transition tables? Your docs say this:

> + <varlistentry>
> + <term><replaceable class="parameter">source_table_name</replaceable></term>
> + <listitem>
> + <para>
> + The name (optionally schema-qualified) of the source table, view or
> + transition table.
> + </para>
> + </listitem>
> + </varlistentry>

But the code says this:

> + /*
> + * XXX if we support transition tables this would need to move earlier
> + * before ExecSetupTransitionCaptureState()
> + */
> + switch (action->commandType)

* Can UPDATEs themselves accept an UPDATE-style WHERE clause, in
addition to the WHEN quals, and the main ON join quals?

I don't see any examples of this in your regression tests.

* You added a new MERGE command tag. Shouldn't there be changes to
libpq's fe-exec.c, to go along with that?

* I noticed this:

> + else if (node->operation == CMD_MERGE)
> + {
> + /*
> + * XXX Add more detailed instrumentation for MERGE changes
> + * when running EXPLAIN ANALYZE?
> + */
> + }

I think that doing better here is necessary.

* I'm not a fan of stored rules, but I still think that this needs to
be discussed:

> - product_queries = fireRules(parsetree,
> + /*
> + * First rule of MERGE club is we don't talk about rules
> + */
> + if (event == CMD_MERGE)
> + product_queries = NIL;
> + else
> + product_queries = fireRules(parsetree,
> result_relation,
> event,
> locks,

* This seems totally unnecessary at first blush:

> + /*
> + * Lock tuple for update.
> + *
> + * XXX Is this really needed? I put this in
> + * just to get hold of the existing tuple.
> + * But if we do need, then we probably
> + * should be looking at the return value of
> + * heap_lock_tuple() and take appropriate
> + * action.
> + */
> + tuple.t_self = *tupleid;
> + test = heap_lock_tuple(relation, &tuple, estate->es_output_cid,
> + lockmode, LockWaitBlock, false, &buffer,
> + &hufd);

* I don't know what the deal is with EPQ and MERGE in general, because
just after the above heap_lock_tuple(), you do this:

> + /*
> + * Test condition, if any
> + *
> + * 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).
> + */
> + if (!ExecQual(action->whenqual, econtext))
> + {
> + if (BufferIsValid(buffer))
> + ReleaseBuffer(buffer);
> + continue;
> + }

Maybe *this* is why you call heap_lock_tuple(), actually, but that's
not clear, and in any case I don't see any point in it if you don't
check heap_lock_tuple()'s return value, and do some other extra thing
on the basis of that return value. No existing heap_lock_tuple()
ignores its return value, and ignoring the return value simply can't
make sense.

Don't WHEN quals need to participate in EPQ reevaluation, in order to
preserve the behavior that we see with UPDATE and DELETE? Why, or why
not?

I suppose that the way you evaluate WHEN quals separately makes a
certain amount of sense, but I think that you need to get things
straight with WHEN quals and READ COMMITTED conflict handling at a
high level (the semantics), which may or may not mean that WHEN quals
participate in EPQ evaluation. If you're going to introduce a special
case, I think you need to note it under the EvalPlanQual section of
the executor README.

This much I'm sure of: you should reevaluate WHEN quals if the UPDATE
chain is walked in READ COMMITTED mode, one way or another. It might
end up happening in your new heap_lock_tuple() retry loop, or you
might do something differently with EPQ, but something should happen
(I haven't got an opinion on the implementation details just yet,
though).

* Your isolation test should be commented. I'd expect you to talk
about what is different about MERGE as far as concurrency goes, if
anything. I note that you don't use additional WHEN quals in your
isolation test at all (just simple WHEN NOT MATCHED + WHEN MATCHED),
which was the first thing I looked for there. Look at
insert-conflict-do-update-3.spec for an example of roughly the kind of
commentary I had hoped to see in your regression test.

> I'm expecting to commit this and then come back for the Partitioning &
> RLS later, but will wait a few days for comments and other reviews.

I don't think that it's okay to defer RLS or partitioning support till
after an initial commit. While it's probably true that MERGE can just
follow ON CONFLICT's example when it comes to column-level privileges,
this won't be true with RLS.

--
Peter Geoghegan