Re: MERGE ... RETURNING

On Wed, 17 Jan 2024 at 14:43, jian he  wrote:
>
> +
> +MERGING (
> property )
> +
> +   The following are valid property values specifying what to return:
> +
> +   
> +    
> +     ACTION
> +     
> +      
> +       The merge action command executed for the current row
> +       ('INSERT', 'UPDATE', or
> +       'DELETE').
> +      
> +     
> +    
> do we change to property?
> Maybe the main para should be two sentences like:
> The merge action command executed for the current row. Possible values
> are: 'INSERT', 'UPDATE',
> 'DELETE'.
>

OK, though actually it should be property.
Also, the parameter should be described as a key word, to match
similar existing keyword function parameters (e.g., the normalize()
function's second parameter).


> + if (!parent_pstate ||
> + parent_pstate->p_expr_kind != EXPR_KIND_MERGE_RETURNING)
> + ereport(ERROR,
> + errcode(ERRCODE_WRONG_OBJECT_TYPE),
> + errmsg("MERGING() can only be used in the RETURNING list of a MERGE command"),
> + parser_errposition(pstate, f->location));
> +
> the object is correct, but not in the right place.
> maybe we should change errcode(ERRCODE_WRONG_OBJECT_TYPE) to
> errcode(ERRCODE_INVALID_OBJECT_DEFINITION)
> also do we need to add some comments explain that why we return it as
> is when it's EXPR_KIND_MERGE_RETURNING.
> (my understanding is that, if key words not match, then it will fail
> at gram.y, like syntax error, else MERGING will based on keywords make
> a MergingFunc node and assign mfop, mftype, location to it)
>

Ah yes, that error code dates back to an earlier version of the patch,
when this check was done in ParseFuncOrColumn(), when I think I just
copied the error code from nearby checks. I agree that
ERRCODE_WRONG_OBJECT_TYPE isn't really right, but I'm not convinced
that ERRCODE_INVALID_OBJECT_DEFINITION is right either, since the
object is valid, but it's not allowed in that part of the query. I
think ERRCODE_SYNTAX_ERROR is probably better (similar to when we find
"DEFAULT" where we shouldn't, for example).


> in src/backend/executor/functions.c
> /*
> * Break from loop if we didn't shut down (implying we got a
> * lazily-evaluated row).  Otherwise we'll press on till the whole
> * function is done, relying on the tuplestore to keep hold of the
> * data to eventually be returned.  This is necessary since an
> * INSERT/UPDATE/DELETE RETURNING that sets the result might be
> * followed by additional rule-inserted commands, and we want to
> * finish doing all those commands before we return anything.
> */
> Does the above comments need to change to INSERT/UPDATE/DELETE/MERGE?
>

No, because MERGE doesn't support tables with rules, so this can't
apply to MERGE. I suppose the comment could be updated to say that,
but I don't think it's worth it, because I think it would distract the
reader from the main point of the comment. I think that function is
complex enough as it is, and since this patch isn't touching it, it
should probably be left alone.


> in src/backend/nodes/nodeFuncs.c
> you add "returningList" to MergeStmt.
> do you need to do the following similar to UpdateStmt, even though
> it's so abstract, i have no idea what's going on.
> `
> if (WALK(stmt->returningList))
> return true;
> `

Ah yes, good point. This can be triggered using a recursive CTE
containing a MERGE ... RETURNING that returns an expression containing
a subquery with a recursive reference to the outer CTE, which should
be an error. I've added a regression test to ensure that this walker
path gets coverage.

Thanks for reviewing. Updated patch attached.

The wider question is whether people are happy with the overall
approach this patch now takes, and the new MERGING() function and
MergingFunc node.

Regards,
Dean