Re: MERGE SQL statement for PG12

On Mon, Jan 14, 2019 at 1:37 AM Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> wrote:
> Can you please help me understand what's fundamentally wrong with the approach and more importantly, can you please explain what would the the architecturally sound way to do this? The same also applies to the executor side where the current approach is deemed wrong, but very little is said on what's the correct way.

I think the basic problem is that optimization should happen in the
optimizer, not the parser. Deciding that MERGE looks like a RIGHT
JOIN is not a parsing task and therefore shouldn't happen in the
parser. The guys who were (are?) proposing support for ALIGN and
NORMALIZE for temporal joins got the same complaint. Here's Tom
talking about that:

http://postgr.es/m/32265.1510681378@sss.pgh.pa.us

I realize that you may have worked around some of the specific issues
that Tom mentions in that email, but I believe the general point
remains valid. What parse analysis should do is construct a
representation of the query the user entered, not some other query
derived from it. And this code clearly does the latter. You can see
by the fact that it uses QSRC_PARSER, for example, a constant that is
currently unused (and for good reason). And you can see that it
results in hacks like the cross-check to make sure that
resultRelRTE->relid == mergeRelRTE->relid. You need that because you
are manipulating and transforming the query too early in the pipeline.
If you do the transformation in the optimizer, you won't need stuff
like this. There are other hints in this function that you're really
doing planning tasks:

+ * The MERGE query's join can be tuned in some cases, see below for these
+ * special case tweaks.

Tuning the query is otherwise known as optimization. Do it in the optimizer.

+ * Simplify the MERGE query as much as possible
+ *
+ * These seem like things that could go into Optimizer, but they are
+ * semantic simplifications rather than optimizations, per se.

Actually, they are optimizations. The fact that an outer join may be
more expensive than an inner join is a costing consideration, not
something that the parser needs to know about.

+ /*
+ * This query should just provide the source relation columns. Later, in
+ * preprocess_targetlist(), we shall also add "ctid" attribute of the
+ * target relation to ensure that the target tuple can be fetched
+ * correctly.
+ */

This is not a problem unique to MERGE. It comes up for UPDATE and
DELETE as well. The handling is not all one place, but it's all in
the optimizer. The place which actually adds the CTID column is in
preprocess_targetlist(), but note that it's much smarter than the code
in the MERGE patch, because it can do different things depending on
which type of rowmark is requested. The code as it exists in the
MERGE patch can't possibly work for anything that doesn't have a CTID
table, which means it won't work for FDWs and, in the future, any new
heap types added via pluggable storage unless they happen to support
CTID. Correctly written, MERGE will leverage what the optimizer
already knows about rowmarks rather than reinventing them in the parse
analysis phase.

You should redesign this whole representation so that you just pass
the whole query through to the optimizer without any structural
change. Just as we do for other statements, you need to do the basic
transformation stuff like looking up relation OIDs: that has to do
with what the query MEANS and the external SQL objects on which it
DEPENDS; those things have to be figured out before planning so that
things like the plan-cache work correctly. But also just as we do for
other statements, you should avoid having any code in this function
that has to do with the way in which the MERGE should ultimately be
executed, and you should not have any code here that builds a new
query or does heavy restructuring of the parse tree.

Since it looks like we will initially have only one strategy for
executing MERGE, it might be OK to move the transformation that you're
currently doing in transformMergeStmt() to some early phase of the
optimizer. Here's me making approximately the same point about ALIGN
and NORMALIZE:

https://www.postgresql.org/message-id/CA+TgmoZ=UkRzpisqK5Qox_ekLG+SQP=xBeFiDkXTgLF_=1FH+Q@mail.gmail.com

However, I'm not sure that's actually the right way to go. Something,
possibly this transformation, is causing us to end up with two copies
of the target relation. This comment is a pretty good clue that this
is nothing but an ugly kludge:

+ * While executing MERGE, the target relation is processed twice; once
+ * as a target relation and once to run a join between the target and the
+ * source. We generate two different RTEs for these two purposes, one with
+ * rte->inh set to false and other with rte->inh set to true.
+ *
+ * Since the plan re-evaluated by EvalPlanQual uses the join RTE, we must
+ * install the updated tuple in the scan corresponding to that RTE. The
+ * following member tracks the index of the second RTE for EvalPlanQual
+ * purposes. ri_mergeTargetRTI is non-zero only when MERGE is in-progress.
+ * We use ri_mergeTargetRTI to run EvalPlanQual for MERGE and
+ * ri_RangeTableIndex elsewhere.

First of all, overloading the rte->inh flag to have anything to do
with this seems dead wrong. It's not a good idea to reuse random
Boolean variables for unrelated purposes. Moreover, I can't see how
this could ever be made to work with partitioned tables if that flag
has already been taken over for something else. Second, the fact that
you need logic to take the EPQ tuple from one scan and stick it into
the other scan strongly suggests to me that there shouldn't be two
scans or two RTEs in the first place.

I want to point out that it is not as if nobody has reviewed this
patch previously. Here is Andres making basically the same point
about parse analysis that I'm making here -- FWIW, I didn't find his
reply until after I'd written the above:

https://www.postgresql.org/message-id/20180403021800.b5nsgiclzanobiup%40alap3.anarazel.de

Here he is explaining these points some more:

https://www.postgresql.org/message-id/20180405200003.gar3j26gsk32gqpe%40alap3.anarazel.de

And here's Peter Geoghegan not only explaining the same problem but
having a go at fixing it:

https://www.postgresql.org/message-id/CAH2-Wz%3DZwNQvp11XjeHo-dBLHr9GDRi1vao8w1j7LQ8mOsUkzw%40mail.gmail.com

Actually, I see that Peter Geoghegan not just the emails above but a
blizzard of other emails explaining the structural problems with the
patch, which I now see include not only the parse analysis concerns
but also the business of multiple RTEs which I mentioned above.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company