Re: MERGE ... WHEN NOT MATCHED BY SOURCE

On Thu, 5 Jan 2023 at 11:03, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> I haven't read this patch other than superficially; I suppose the
> feature it's introducing is an OK one to have as an extension to the
> standard.  (I hope the community members that are committee members
> will propose this extension to become part of the standard.)
>

Thanks for looking!

> > --- a/src/backend/optimizer/prep/preptlist.c
> > +++ b/src/backend/optimizer/prep/preptlist.c
> > @@ -157,15 +157,14 @@ preprocess_targetlist(PlannerInfo *root)
> >                       /*
> >                        * Add resjunk entries for any Vars used in each action's
> >                        * targetlist and WHEN condition that belong to relations other
> > -                      * than target.  Note that aggregates, window functions and
> > -                      * placeholder vars are not possible anywhere in MERGE's WHEN
> > -                      * clauses.  (PHVs may be added later, but they don't concern us
> > -                      * here.)
> > +                      * than target.  Note that aggregates and window functions are not
> > +                      * possible anywhere in MERGE's WHEN clauses, but PlaceHolderVars
> > +                      * may have been added by subquery pullup.
> >                        */
> >                       vars = pull_var_clause((Node *)
> >                                                                  list_concat_copy((List *) action->qual,
> >
action->targetList),
> > -                                                                0);
> > +                                                                PVC_INCLUDE_PLACEHOLDERS);
>
> Hmm, is this new because of NOT MATCHED BY SOURCE, or is it something
> that can already be hit by existing features of MERGE?  In other words
> -- is this a bug fix that should be backpatched ahead of introducing NOT
> MATCHED BY SOURCE?
>

It's new because of NOT MATCHED BY SOURCE, and I also found that I had
to make the same change in the MERGE INTO view patch, in the case
where the target view is simple enough to allow subquery pullup, but
also had INSTEAD OF triggers causing the pullup to happen in the
planner rather than the rewriter.

I couldn't think of a way that it could happen with the existing MERGE
code though, so I don't think it's a bug that needs fixing and
back-patching.

> > @@ -127,10 +143,12 @@ transformMergeStmt(ParseState *pstate, M
> >        */
> >       is_terminal[0] = false;
> >       is_terminal[1] = false;
> > +     is_terminal[2] = false;
>
> I think these 0/1/2 should be replaced by the values of MergeMatchKind.
>

Agreed.

> > +     /* Join type required */
> > +     if (left_join && right_join)
> > +             qry->mergeJoinType = JOIN_FULL;
> > +     else if (left_join)
> > +             qry->mergeJoinType = JOIN_LEFT;
> > +     else if (right_join)
> > +             qry->mergeJoinType = JOIN_RIGHT;
> > +     else
> > +             qry->mergeJoinType = JOIN_INNER;
>
> One of the review comments that MERGE got initially was that parse
> analysis was not a place to "do query optimization", in the sense that
> the original code was making a decision whether to make an outer or
> inner join based on the set of WHEN clauses that appear in the command.
> That's how we ended up with transform_MERGE_to_join and
> mergeUseOuterJoin instead.  This new code is certainly not the same, but
> it makes me a bit unconfortable.  Maybe it's OK, though.
>

Yeah I agree, it's a bit ugly. Perhaps a better solution would be to
do away with that field entirely and just make the decision in
transform_MERGE_to_join() by examining the action list again. That
would require making MergeAction's "matched" field a MergeMatchKind
rather than a bool, but maybe that's not so bad, since retaining that
information might prove useful one day.

Regards,
Dean