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