Feedback-ID: ia2694551:Fastmail
Date: Wed, 4 Jan 2023 12:57:58 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: MERGE ... WHEN NOT MATCHED BY SOURCE
Message-ID: <20230104115758.2ajc235gphwfe3d6@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAEZATCUm0yku11PQ61d2M92sWnLJaRqvrMfrcmsLCeap1sSdEA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230104115758.2ajc235gphwfe3d6%40alvherre.pgsql>
Precedence: bulk

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.)

On 2023-Jan-02, Dean Rasheed wrote:

> --- 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?

> @@ -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.

> +	/* 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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/