Re: ask for review of MERGE

Boxuan Zhai wrote:
> Yes, it should be NULL instead of NIL.

OK, I applied that patch to my local copy and pushed it out to github:

http://github.com/greg2ndQuadrant/postgres/commit/9013ba9e81490e3623add1b029760817021297c0

That represents what I tested against. However, when I tried to merge
against HEAD once I was finished, I discovered this patch has bit-rotted
significantly. If you have a local copy that works for you, I would not
recommend pulling in the PostgreSQL repo updates done in the last couple
of weeks yet. Friday's "Allow WITH clauses to be attached to INSERT,
UPDATE, DELETE statements" commit in particular conflicts quite a bit
with your changes. Attached is a rebased patch that applies to HEAD now
after a round of fixes to resolve those. But it doesn't work yet,
because of recent change to the ExecUpdate and ExecDelete functions
you're calling from src/backend/executor/nodeModifyTable.c inside
ExecMerge. If you can continue working on the patch without merging
recent repo work, I can hack away at fixing that once I figure out what
got changed there recently. It's taken some painful git work to sort
out what I've done so far, there's more left to do, and I know that's
not an area you specialize in.

Back to the feature review...I dove into how I expected this to work,
relative to what it actually does at the moment. That didn't really go
too well so far, but I don't know that this represents any fundamental
issue with the patch. Just situations the existing code didn't really
anticipate we have to flush out. As a general community FYI here, while
it's taken me a while to get up to speed on this whole feature, I expect
to keep chugging away on this regardless of the CommitFest boundaries.
This feature is both too big and too important to just stop working on
it because a date has passed.

Onto the test cases. The examples that Boxuan has been working with,
and that the regression tests included with the patch exercise, all
involve two tables being joined together using MERGE. The use case I
decided to test instead was when you're trying to simulate an UPSERT
where only a single table is involved. I couldn't get to this to work
correctly. Maybe I'm just using MERGE wrong here, but I tried so many
different variations without success (including one that's basically
copied from Simon's original regression test set suggestions) that I
suspect there may be a subtle problem with the implementation instead.

To replicate the most straightforward variations of what I ran into, you
can start with the same data that's populated by the regression test set:

CREATE TABLE Stock(item_id int UNIQUE, balance int);
INSERT INTO Stock VALUES (10, 2200);
INSERT INTO Stock VALUES (20, 1900);
SELECT * FROM Stock;

item_id | balance
---------+---------
10 | 2200
20 | 1900

If you now execute the following:

MERGE INTO Stock t
USING (SELECT * FROM Stock WHERE item_id=10) AS s
ON s.item_id=t.item_id
WHEN MATCHED THEN UPDATE SET balance=s.balance + 1
WHEN NOT MATCHED THEN INSERT VALUES (10,1)
;

This works fine, and updates the matching row:

item_id | balance
---------+---------
20 | 1900
10 | 2201

But if I give it a key that doesn't exist instead:

MERGE INTO Stock t
USING (SELECT * FROM Stock WHERE item_id=30) AS s
ON s.item_id=t.item_id
WHEN MATCHED THEN UPDATE SET balance=s.balance + 1
WHEN NOT MATCHED THEN INSERT VALUES (30,1)
;

This doesn't execute the NOT MATCHED case and INSERT the way I expected
it to. It just gives back "MERGE 0".

Since I wasn't sure if the whole "subquery in the USING clause" case was
really implemented fully, I then tried to do this with something more
like the working regression test examples. I expected this to do the
same thing as the first example:

MERGE INTO Stock t
USING Stock s
ON s.item_id=10 AND s.item_id=t.item_id
WHEN MATCHED THEN UPDATE SET balance=s.balance + 1
WHEN NOT MATCHED THEN INSERT VALUES (10,1)
;

But it gives back this:

ERROR: duplicate key value violates unique constraint "stock_item_id_key"
DETAIL: Key (item_id)=(10) already exists.

Can't tell from that whether it's hitting the MATCHED or NOT MATCHED
side of things to generate that. But it doesn't work any better if you
give it an example that doesn't exist:

MERGE INTO Stock t
USING Stock s
ON s.item_id=30 AND s.item_id=t.item_id
WHEN MATCHED THEN UPDATE SET balance=s.balance + 1
WHEN NOT MATCHED THEN INSERT VALUES (30,1)
;

ERROR: duplicate key value violates unique constraint "stock_item_id_key"
DETAIL: Key (item_id)=(30) already exists.

Now that one is really weird, because that key value certainly doesn't
exist yet in the table. There seem to be a couple of issues in the area
of joining a table with itself here. Feel free to tell me I just don't
know how to use MERGE if that's the case in any of these.

The other thing I noticed that may take some work to sort out is that I
haven't had any luck getting MERGE to execute from within a plpgsql
function. I was hoping I could use this to update the pgbench tables:

CREATE OR REPLACE FUNCTION merge_account_balance(key INT, delta NUMERIC)
RETURNS VOID AS
$$
BEGIN
MERGE INTO pgbench_accounts t USING (SELECT * FROM pgbench_accounts
WHERE aid = key) AS s ON t.aid=s.aid WHEN MATCHED THEN UPDATE SET
abalance = s.abalance + delta WHEN NOT MATCHED THEN INSERT VALUES
(key,1+(key / 100000)::integer,delta,'');
END;
$$
LANGUAGE plpgsql;

But I just get this instead:

ERROR: "pgbench_accounts" is not a known variable
LINE 4: MERGE INTO pgbench_accounts t USING (SELECT * FROM p...

The other way I wrote the MERGE statement above (not using a subquery)
does the same thing. I know that error messages is coming from the
changes introduced in
http://archives.postgresql.org/pgsql-committers/2010-01/msg00161.php but
I'm not familiar enough with the whole grammar implementation to know
what that means yet.

That's what I found so far in my second pass over this. Once the first
problem here is sorted out, I've already worked out how to test the
performance of the code using pgbench. Have all the scripts ready to go
once the correct MERGE statement is plugged into them, just ran into
this same class of problems when I tried them. So far I was only able
to see how fast the UPDATE path worked though, which isn't very helpful
yet. My hope here is to test the MERGE implementation vs. the classic
pl/pgsql implementation of UPSERT, calling both within a function so
it's a fair comparison, and see how that goes. This may flush out
concurrency bugs that are in the MERGE code as well.

--
Greg Smith, 2ndQuadrant US greg(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services and Support www.2ndQuadrant.us