Re: MERGE Specification

On 11/08/10 11:11, Boxuan Zhai wrote:
> The new patch is done. I named it as merge_v102. (1 means it is the
> non-inheritance merge command, 02 means this is the second time of fixing
> reported bugs)

Thanks. I went through this, fixing all the spurious end-of-line
whitespace first with "git apply --whitespace=fix", and then manually
fixing comment and whitespace style, typos, and doing some other small
comment editing. Resulting patch attached. It is also available at my
git repository at
git://git.postgresql.org/git/users/heikki/postgres.git, branch
'mergestmt'. Please base any further patch versions on this patch, so
that we don't need to redo this cleanup.

I'll continue reviewing this sometime next week, but here's few
miscellaneous issues for starters;

* Explain output of actions needs some work:

> Merge (cost=246.37..272.96 rows=1770 width=38)
> ACTION: UPDATE WHEN NOT MATCHED
> ACTION: INSERT WHEN NOT MATCHED
> -> Merge Left Join (cost=246.37..272.96 rows=1770 width=38)

Should print out more details of the action, like for normal
updates/inserts/deletes. And all uppercase doesn't fit the surrounding
style.

* Is the behavior now SQL-compliant? We had long discussions about the
default action being insert or do nothing or raise error, but I never
got a clear picture of what the SQL spec says and whether we're compliant.

* What does the "one tuple is error" notice mean? We'll have to do
something about that.. I don't think we've properly thought through the
meaning of RAISE ERROR. Let's cut it out from the patch until then. It's
only a few dozen lines to put back when we know what to do about it.

* Do you need the MergeInsert/MergeUpdate/MergeDelete alias nodes? Would
it be simpler to just use InsertStmt/UpdateStmt/DeleteStmt directly?

* Do you need the out-function for DeleteStmt? Why not for UpdateStmt
and InsertStmt?

* I wonder if it would be simpler to check the fact that you can only
have UPDATE/DELETE as a WHEN MATCHED action and only INSERT as a WHEN
NOT MATCHED action directly in the grammar?

* Regarding this speculation in the MergeStmt grammar rule:
> /*although we have only one USING table,
> we still make it a list, maybe in future
> we will allow multiple USING tables.*/

I wonder what the semantics of having multiple USING tables would be? If
it would be like "USING (SELECT * FROM a UNION ALL SELECT * FROM b)",
then we don't ever need it because you can already achieve it with that
subquery. If it's something like "USING (SELECT * FROM a,b WHERE ...)",
then we again don't need it because you can write that instead. So I
think we should give up on the notion that source can be a list of
tables, and simplify the code everywhere accordingly.

* Instead of scanning the list of actions in ExecBS/ExecASMergeTriggers
every time, should set flags at plan time to mark what kind of actions
there is in the statement. Or should we defer firing the statement
triggers until we hit the first matching row and execute the action for
the first time?

* Do we need the 'replaced' field? Could you just replace the action
with a DO NOTHING action instead.

* This crashes:

postgres=# CREATE TABLE target AS (SELECT 1 AS id);
SELECT 1
postgres=# MERGE into target t
USING (select 1 AS id) AS s
ON t.id = s.id
WHEN MATCHED THEN
UPDATE SET id = (SELECT COUNT(*) FROM generate_series(1,10))
;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com