Re: support for MERGE

On 2022-Jan-28, Andres Freund wrote:

> Any chance you could split this into something more reviewable? The overall
> diff stats are: 102 files changed, 8589 insertions(+), 234 deletions(-) thats
> pretty hard to really review. Incremental commits don't realy help with that.

I'll work on splitting this next week.

> One thing from skimming: There's not enough documentation about the approach
> imo - it's a complicated enough feature to deserve more than the one paragraph
> in src/backend/executor/README.

Sure, I'll have a look at that.

> I'm still [1],[2] uncomfortable with execMerge.c kinda-but-not-really being an
> executor node.

I think we should make a decision on code arrangement here.  From my
perspective, MERGE isn't its own executor node; rather it's just another
"method" in ModifyTable.  Which makes sense, given that all it does is
call parts of INSERT, UPDATE, DELETE which are the other ModifyTable
methods.  Having a separate file doesn't strike me as great, but on the
other hand it's true that merely moving all the execMerge.c code into
nodeModifyTable.c makes the latter too large.  However I don't want to
create a .h file that means exposing all those internal functions to the
outside world.  My ideal would be to have each INSERT, UPDATE, DELETE,
MERGE as its own separate .c file, which would be #included from
nodeModifyTable.c.  We don't use that pattern anywhere though.  Any
opposition to that?  (The prototypes for each file would have to live in
nodeModifyTable.c itself.)


> > diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
> > index 1a9c1ac290..280ac40e63 100644
> > --- a/src/backend/commands/trigger.c
> > +++ b/src/backend/commands/trigger.c
> 
> This stuff seems like one candidate for splitting out.

Yeah, I had done that.  It's now posted as 0001.

> > +	/*
> > +	 * We maintain separate transition tables for UPDATE/INSERT/DELETE since
> > +	 * MERGE can run all three actions in a single statement. Note that UPDATE
> > +	 * needs both old and new transition tables whereas INSERT needs only new,
> > +	 * and DELETE needs only old.
> > +	 */
> > +
> > +	/* "old" transition table for UPDATE, if any */
> > +	Tuplestorestate *old_upd_tuplestore;
> > +	/* "new" transition table for UPDATE, if any */
> > +	Tuplestorestate *new_upd_tuplestore;
> > +	/* "old" transition table for DELETE, if any */
> > +	Tuplestorestate *old_del_tuplestore;
> > +	/* "new" transition table INSERT, if any */
> > +	Tuplestorestate *new_ins_tuplestore;
> > +
> >  	TupleTableSlot *storeslot;	/* for converting to tuplestore's format */
> >  };
> 
> Do we need to do something slightly clever about the memory limits for the
> tuplestore? Each of them having a separate work_mem makes nodeModifyTable.c
> one memory hungry node (the worst of all maybe?).

Not sure how that would work.  The memory handling is inside
tuplestore.c IIRC ... I think that would require some largish
refactoring.  Merely dividing by four doesn't seem like a great answer
either.  Perhaps we could divide by two and be optimistic about it.

> > +				/*
> > +				 * Project the tuple.  In case of a partitioned table, the
> > +				 * projection was already built to use the root's descriptor,
> > +				 * so we don't need to map the tuple here.
> > +				 */
> > +				actionInfo.actionState = action;
> > +				insert_slot = ExecProject(action->mas_proj);
> > +
> > +				(void) ExecInsert(mtstate, rootRelInfo,
> > +								  insert_slot, slot,
> > +								  estate, &actionInfo,
> > +								  mtstate->canSetTag);
> > +				InstrCountFiltered1(&mtstate->ps, 1);
> 
> What happens if somebody concurrently inserts a conflicting row?

An open question to which I don't have any good answer RN.

-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)