Subject: Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)

From: Dean Rasheed <[email-masked]>
Date: Wed, 28 Feb 2024 12:11:02 +0000

On Wed, 28 Feb 2024 at 09:16, jian he <[email-masked]> wrote:
>
> + oldcontext = MemoryContextSwitchTo(estate->es_query_cxt);
> +
> + node->as_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod);
> +
> + ExecAssignExprContext(estate, &node->ps);
> +
> + node->ps.ps_ProjInfo =
> + ExecBuildProjectionInfo(castNode(Append, node->ps.plan)->epq_targetlist,
> +
> EvalPlanQualStart, EvalPlanQualNext will switch the memory context to
> es_query_cxt.
> so the memory context switch here is not necessary?
>

Yes it is necessary. The EvalPlanQual mechanism switches to the
epqstate->recheckestate->es_query_cxt memory context, which is not the
same as the main query's estate->es_query_cxt (they're different
executor states). Most stuff allocated under EvalPlanQual() is
intended to be short-lived (just for the duration of that specific EPQ
check), whereas this stuff (the TupleDesc and Projection) is intended
to last for the duration of the main query, so that it can be reused
in later EPQ checks.

> do you think it's sane to change
> `ExecAssignExprContext(estate, &node->ps);`
> to
> `
> /* need an expression context to do the projection */
> if (node->ps.ps_ExprContext == NULL)
> ExecAssignExprContext(estate, &node->ps);
> `
> ?

Possibly. More importantly, it should be doing a ResetExprContext() to
free any previous stuff before projecting the new row.

At this stage, this is just a rough draft patch. There are many things
like that and code comments that will need to be improved before it is
committable, but for now I'm more concerned with whether it actually
works, and if the approach it's taking is sane.

I've tried various things and I haven't been able to break it, but I'm
still nervous that I might be overlooking something, particularly in
relation to what might get added to the targetlist at various stages
during planning for different types of query.

Regards,
Dean

---

From: jian he <[email-masked]>
Date: Thu, 29 Feb 2024 11:04:35 +0800

> to last for the duration of the main query, so that it can be reused
sorry for the noise. I understand it now.

Another small question:
for the Append case, we can set/initialize it at create_append_plan,
all other elements are initialized there,
why we set it at set_append_references.
just wondering.

---

From: "zwj" <[email-masked]>
Date: Tue, 5 Mar 2024 18:04:33 +0800

Hi，hackers
&nbsp;
&nbsp;I may have discovered another issue in the concurrency scenario of merge,&nbsp; and I am currently not sure if this new issue is related to the previous one.&nbsp;
&nbsp;It seems that it may also be an issue with the EPQ mechanism in the merge scenario?&nbsp;
&nbsp;I will provide this test case, hoping it will be helpful for you to fix related issues in the future.

&nbsp;&nbsp;&nbsp; DROP TABLE IF EXISTS src1, tgt;
&nbsp;&nbsp;&nbsp; CREATE TABLE src1 (a int, b text);
&nbsp;&nbsp;&nbsp; CREATE TABLE tgt (a int, b text);
&nbsp;&nbsp;&nbsp; INSERT INTO src1 SELECT x, 'Src1 '||x FROM generate_series(1, 3) g(x);
&nbsp;&nbsp;&nbsp; INSERT INTO tgt SELECT x, 'Tgt '||x FROM generate_series(1, 6, 2) g(x);
&nbsp;&nbsp;&nbsp; insert into src1 values(3,'src1 33');

&nbsp;If I only execute merge , I will get the following error:
&nbsp;&nbsp;&nbsp; merge into tgt a using src1 c on&nbsp; a.a = c.a when matched then update set b = c.b when not matched then insert (a,b) values(c.a,c.b);&nbsp; -- excute fail
&nbsp;&nbsp;&nbsp; ERROR:&nbsp; MERGE command cannot affect row a second time
&nbsp;&nbsp;&nbsp; HIINT:&nbsp; Ensure that not more than one source row matches any one target row.

&nbsp;But when I execute the update and merge concurrently, I will get the following result set.

&nbsp; --session1
&nbsp;&nbsp;&nbsp; begin;

&nbsp;&nbsp;&nbsp; update tgt set b = 'tgt333' where a =3;

&nbsp; --session2
&nbsp;&nbsp;&nbsp; merge into tgt a using src1 c on&nbsp; a.a = c.a when matched then update set  b = c.b when not matched then insert (a,b) values(c.a,c.b);&nbsp; -- excute success
&nbsp; --session1
&nbsp;&nbsp;&nbsp; commit;
&nbsp;&nbsp;&nbsp; select * from tgt;
&nbsp;&nbsp; &nbsp; a |&nbsp;&nbsp;&nbsp; b&nbsp;&nbsp; &nbsp;
&nbsp;&nbsp;&nbsp; ---+---------
&nbsp;&nbsp;&nbsp;&nbsp; 5 | Tgt 5
&nbsp;&nbsp;&nbsp;&nbsp; 1 | Src1 1
&nbsp;&nbsp;&nbsp;&nbsp; 2 | Src1 2
&nbsp;&nbsp;&nbsp;&nbsp; 3 | Src1 3
&nbsp;&nbsp;&nbsp;&nbsp; 3 | src1 33

&nbsp; I think even if the tuple with id:3 is udpated, merge should still be able to retrieve new tuples with id:3,&nbsp; and&nbsp;report the same error as above?

Regards,
wenjiang zhang

------------------&nbsp;原始邮件&nbsp;------------------
发件人:                                                                                                                        "jian he"                                                                                    <[email-masked]&gt;;
发送时间:&nbsp;2024年2月29日(星期四) 中午11:04
收件人:&nbsp;"Dean Rasheed"<[email-masked]&gt;;
抄送:&nbsp;"Tom Lane"<[email-masked]&gt;;"zwj"<[email-masked]&gt;;"pgsql-hackers"<[email-masked]&gt;;
主题:&nbsp;Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)

On Wed, Feb 28, 2024 at 8:11 PM Dean Rasheed <[email-masked]&gt; wrote:
&gt;
&gt; On Wed, 28 Feb 2024 at 09:16, jian he <[email-masked]&gt; wrote:
&gt; &gt;
&gt; &gt; + oldcontext = MemoryContextSwitchTo(estate-&gt;es_query_cxt);
&gt; &gt; +
&gt; &gt; + node-&gt;as_epq_tupdesc = lookup_rowtype_tupdesc_copy(tupType, tupTypmod);
&gt; &gt; +
&gt; &gt; + ExecAssignExprContext(estate, &amp;node-&gt;ps);
&gt; &gt; +
&gt; &gt; + node-&gt;ps.ps_ProjInfo =
&gt; &gt; + ExecBuildProjectionInfo(castNode(Append, node-&gt;ps.plan)-&gt;epq_targetlist,
&gt; &gt; +
&gt; &gt; EvalPlanQualStart, EvalPlanQualNext will switch the memory context to
&gt; &gt; es_query_cxt.
&gt; &gt; so the memory context switch here is not necessary?
&gt; &gt;
&gt;
&gt; Yes it is necessary. The EvalPlanQual mechanism switches to the
&gt; epqstate-&gt;recheckestate-&gt;es_query_cxt memory context, which is not the
&gt; same as the main query's estate-&gt;es_query_cxt (they're different
&gt; executor states). Most stuff allocated under EvalPlanQual() is
&gt; intended to be short-lived (just for the duration of that specific EPQ
&gt; check), whereas this stuff (the TupleDesc and Projection) is intended
&gt; to last for the duration of the main query, so that it can be reused
&gt; in later EPQ checks.
&gt;
sorry for the noise. I understand it now.

Another small question:

---

From: Dean Rasheed <[email-masked]>
Date: Tue, 5 Mar 2024 13:55:07 +0000

[cc'ing Alvaro]

On Tue, 5 Mar 2024 at 10:04, zwj <[email-masked]> wrote:
>
>  If I only execute merge , I will get the following error:
>     merge into tgt a using src1 c on  a.a = c.a when matched then update set b = c.b when not matched then insert (a,b) values(c.a,c.b);  -- excute fail
>     ERROR:  MERGE command cannot affect row a second time
>     HIINT:  Ensure that not more than one source row matches any one target row.
>
>  But when I execute the update and merge concurrently, I will get the following result set.
>

Yes, this should still produce that error, even after a concurrent update.

In the first example without a concurrent update, when we reach the
tgt.a = 3 row the second time, ExecUpdateAct() returns TM_SelfModified
and we do this:

    case TM_SelfModified:

        /*
         * The SQL standard disallows this for MERGE.
         */
        if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
            ereport(ERROR,
                    (errcode(ERRCODE_CARDINALITY_VIOLATION),
            /* translator: %s is a SQL command name */
                     errmsg("%s command cannot affect row a second time",
                            "MERGE"),
                     errhint("Ensure that not more than one source row
matches any one target row.")));
        /* This shouldn't happen */
        elog(ERROR, "attempted to update or delete invisible tuple");
        break;

whereas in the second case, after a concurrent update, ExecUpdateAct()
returns TM_Updated, we attempt to lock the tuple prior to running EPQ,
and table_tuple_lock() returns TM_SelfModified, which does this:

            case TM_SelfModified:

                /*
                 * This can be reached when following an update
                 * chain from a tuple updated by another session,
                 * reaching a tuple that was already updated in
                 * this transaction. If previously modified by
                 * this command, ignore the redundant update,
                 * otherwise error out.
                 *
                 * See also response to TM_SelfModified in
                 * ExecUpdate().
                 */
                if (context->tmfd.cmax != estate->es_output_cid)
                    ereport(ERROR,
                            (errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
                             errmsg("tuple to be updated or deleted
was already modified by an operation triggered by the current
command"),
                             errhint("Consider using an AFTER trigger
instead of a BEFORE trigger to propagate changes to other rows.")));
                return false;

My initial reaction is that neither of those blocks of code is
entirely correct, and that they should both be doing both of those
checks. I.e., something like the attached (which probably needs some
additional test cases).

Regards,
Dean
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
new file mode 100644
index fcb6133..9351fbc
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -3001,8 +3001,29 @@ lmerge_matched:
 			case TM_SelfModified:
 
 				/*
-				 * The SQL standard disallows this for MERGE.
+				 * The target tuple was already updated or deleted by the
+				 * current command, or by a later command in the current
+				 * transaction.  The former case is explicitly disallowed by
+				 * the SQL standard for MERGE, which insists that the MERGE
+				 * join condition should not join a target row to more than
+				 * one source row.
+				 *
+				 * The latter case arises if the tuple is modified by a
+				 * command in a BEFORE trigger, or perhaps by a command in a
+				 * volatile function used in the query.  In such situations we
+				 * should not ignore the MERGE action, but it is equally
+				 * unsafe to proceed.  We don't want to discard the original
+				 * MERGE action while keeping the triggered actions based on
+				 * it; and it would be no better to allow the original MERGE
+				 * action while discarding the updates that it triggered.  So
+				 * throwing an error is the only safe course.
 				 */
+				if (context->tmfd.cmax != estate->es_output_cid)
+					ereport(ERROR,
+							(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
+							 errmsg("tuple to be updated or deleted was already modified by an operation triggered by the current command"),
+							 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
 				if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
 					ereport(ERROR,
 							(errcode(ERRCODE_CARDINALITY_VIOLATION),
@@ -3010,6 +3031,7 @@ lmerge_matched:
 							 errmsg("%s command cannot affect row a second time",
 									"MERGE"),
 							 errhint("Ensure that not more than one source row matches any one target row.")));
+
 				/* This shouldn't happen */
 				elog(ERROR, "attempted to update or delete invisible tuple");
 				break;
@@ -3118,19 +3140,27 @@ lmerge_matched:
 							/*
 							 * This can be reached when following an update
 							 * chain from a tuple updated by another session,
-							 * reaching a tuple that was already updated in
-							 * this transaction. If previously modified by
-							 * this command, ignore the redundant update,
-							 * otherwise error out.
-							 *
-							 * See also response to TM_SelfModified in
-							 * ExecUpdate().
+							 * reaching a tuple that was already updated or
+							 * deleted by the current command, or by a later
+							 * command in the current transaction. As above,
+							 * this should always be treated as an error.
 							 */
 							if (context->tmfd.cmax != estate->es_output_cid)
 								ereport(ERROR,
 										(errcode(ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION),
 										 errmsg("tuple to be updated or deleted was already modified by an operation triggered by the current command"),
 										 errhint("Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.")));
+
+							if (TransactionIdIsCurrentTransactionId(context->tmfd.xmax))
+								ereport(ERROR,
+										(errcode(ERRCODE_CARDINALITY_VIOLATION),
+								/* translator: %s is a SQL command name */
+										 errmsg("%s command cannot affect row a second time",
+												"MERGE"),
+										 errhint("Ensure that not more than one source row matches any one target row.")));
+
+							/* This shouldn't happen */
+							elog(ERROR, "attempted to update or delete invisible tuple");
 							return false;
 
 						default:

---

From: Dean Rasheed <[email-masked]>
Date: Thu, 7 Mar 2024 10:20:41 +0000

> checks. I.e., something like the attached (which probably needs some

OK, I've pushed and back-patched that fix for this issue, after adding
some tests (nice catch, by the way!).

That wasn't related to the original issue though, so the problem with
UNION ALL still remains to be fixed. The patch from [1] looks
promising (for HEAD at least), but it really needs more pairs of eyes
on it (bearing in mind that it's just a rough proof-of-concept patch
at this stage).

[1] https://www.postgresql.org/message-id/CAEZATCVa-mgPuOdgd8%2BYVgOJ4wgJuhT2mJznbj_tmsGAP8hHJw%40mail.gmail.com

Regards,
Dean