MIME-Version: 1.0
References: <tencent_1862D94C23E71924A878C3D107FBDFE6C60A@qq.com>
 <CAEZATCUyvLVU1bon7E5LExR+YFjFm1AJbGdrkVXx5oVB_LVE3g@mail.gmail.com>
 <CAEZATCWPByO0ovyV_fU+hy5vDu=d+JW3GLdBF_S4zrfADBGvYQ@mail.gmail.com>
 <551454.1708618854@sss.pgh.pa.us> <711158.1708647157@sss.pgh.pa.us>
 <CAEZATCVa-mgPuOdgd8+YVgOJ4wgJuhT2mJznbj_tmsGAP8hHJw@mail.gmail.com>
 <CACJufxEYuCnGUUymGXu-V742OE2+BGx18-znHnWkCuN+gN19eA@mail.gmail.com>
 <CAEZATCUs17W0O78=LLnZSAuN2vq2myxuOtMFgkMsYR-JKHRmSA@mail.gmail.com>
 <CACJufxFfbDhKiNHs8+pnZcZMtOHiANcXzV=JG_Qf=bhi6=vL7w@mail.gmail.com> <tencent_41DE0FF443FE14B94A5898D373792109E408@qq.com>
In-Reply-To: <tencent_41DE0FF443FE14B94A5898D373792109E408@qq.com>
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Tue, 5 Mar 2024 13:55:07 +0000
Message-ID: <CAEZATCVCFDWQXyfBgcO8HxoH22aV1kKbBnJhRY7+wWDLRCMV2w@mail.gmail.com>
Subject: Re: bug report: some issues about pg_15_stable(8fa4a1ac61189efffb8b851ee77e1bc87360c445)
To: zwj <sxzwj@vip.qq.com>
Cc: jian he <jian.universality@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, 
	pgsql-hackers <pgsql-hackers@lists.postgresql.org>, 
	Alvaro Herrera <alvherre@alvh.no-ip.org>
Content-Type: multipart/mixed; boundary="00000000000050f8f80612ea316a"
Archived-At: <https://www.postgresql.org/message-id/CAEZATCVCFDWQXyfBgcO8HxoH22aV1kKbBnJhRY7%2BwWDLRCMV2w%40mail.gmail.com>
Precedence: bulk

--00000000000050f8f80612ea316a
Content-Type: text/plain; charset="UTF-8"

[cc'ing Alvaro]

On Tue, 5 Mar 2024 at 10:04, zwj <sxzwj@vip.qq.com> 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

--00000000000050f8f80612ea316a
Content-Type: text/x-patch; charset="US-ASCII"; name="merge-self-modified-tuple.patch"
Content-Disposition: attachment; filename="merge-self-modified-tuple.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_ltefby530>
X-Attachment-Id: f_ltefby530

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVNb2RpZnlUYWJsZS5jIGIvc3Jj
L2JhY2tlbmQvZXhlY3V0b3Ivbm9kZU1vZGlmeVRhYmxlLmMKbmV3IGZpbGUgbW9kZSAxMDA2NDQK
aW5kZXggZmNiNjEzMy4uOTM1MWZiYwotLS0gYS9zcmMvYmFja2VuZC9leGVjdXRvci9ub2RlTW9k
aWZ5VGFibGUuYworKysgYi9zcmMvYmFja2VuZC9leGVjdXRvci9ub2RlTW9kaWZ5VGFibGUuYwpA
QCAtMzAwMSw4ICszMDAxLDI5IEBAIGxtZXJnZV9tYXRjaGVkOgogCQkJY2FzZSBUTV9TZWxmTW9k
aWZpZWQ6CiAKIAkJCQkvKgotCQkJCSAqIFRoZSBTUUwgc3RhbmRhcmQgZGlzYWxsb3dzIHRoaXMg
Zm9yIE1FUkdFLgorCQkJCSAqIFRoZSB0YXJnZXQgdHVwbGUgd2FzIGFscmVhZHkgdXBkYXRlZCBv
ciBkZWxldGVkIGJ5IHRoZQorCQkJCSAqIGN1cnJlbnQgY29tbWFuZCwgb3IgYnkgYSBsYXRlciBj
b21tYW5kIGluIHRoZSBjdXJyZW50CisJCQkJICogdHJhbnNhY3Rpb24uICBUaGUgZm9ybWVyIGNh
c2UgaXMgZXhwbGljaXRseSBkaXNhbGxvd2VkIGJ5CisJCQkJICogdGhlIFNRTCBzdGFuZGFyZCBm
b3IgTUVSR0UsIHdoaWNoIGluc2lzdHMgdGhhdCB0aGUgTUVSR0UKKwkJCQkgKiBqb2luIGNvbmRp
dGlvbiBzaG91bGQgbm90IGpvaW4gYSB0YXJnZXQgcm93IHRvIG1vcmUgdGhhbgorCQkJCSAqIG9u
ZSBzb3VyY2Ugcm93LgorCQkJCSAqCisJCQkJICogVGhlIGxhdHRlciBjYXNlIGFyaXNlcyBpZiB0
aGUgdHVwbGUgaXMgbW9kaWZpZWQgYnkgYQorCQkJCSAqIGNvbW1hbmQgaW4gYSBCRUZPUkUgdHJp
Z2dlciwgb3IgcGVyaGFwcyBieSBhIGNvbW1hbmQgaW4gYQorCQkJCSAqIHZvbGF0aWxlIGZ1bmN0
aW9uIHVzZWQgaW4gdGhlIHF1ZXJ5LiAgSW4gc3VjaCBzaXR1YXRpb25zIHdlCisJCQkJICogc2hv
dWxkIG5vdCBpZ25vcmUgdGhlIE1FUkdFIGFjdGlvbiwgYnV0IGl0IGlzIGVxdWFsbHkKKwkJCQkg
KiB1bnNhZmUgdG8gcHJvY2VlZC4gIFdlIGRvbid0IHdhbnQgdG8gZGlzY2FyZCB0aGUgb3JpZ2lu
YWwKKwkJCQkgKiBNRVJHRSBhY3Rpb24gd2hpbGUga2VlcGluZyB0aGUgdHJpZ2dlcmVkIGFjdGlv
bnMgYmFzZWQgb24KKwkJCQkgKiBpdDsgYW5kIGl0IHdvdWxkIGJlIG5vIGJldHRlciB0byBhbGxv
dyB0aGUgb3JpZ2luYWwgTUVSR0UKKwkJCQkgKiBhY3Rpb24gd2hpbGUgZGlzY2FyZGluZyB0aGUg
dXBkYXRlcyB0aGF0IGl0IHRyaWdnZXJlZC4gIFNvCisJCQkJICogdGhyb3dpbmcgYW4gZXJyb3Ig
aXMgdGhlIG9ubHkgc2FmZSBjb3Vyc2UuCiAJCQkJICovCisJCQkJaWYgKGNvbnRleHQtPnRtZmQu
Y21heCAhPSBlc3RhdGUtPmVzX291dHB1dF9jaWQpCisJCQkJCWVyZXBvcnQoRVJST1IsCisJCQkJ
CQkJKGVycmNvZGUoRVJSQ09ERV9UUklHR0VSRURfREFUQV9DSEFOR0VfVklPTEFUSU9OKSwKKwkJ
CQkJCQkgZXJybXNnKCJ0dXBsZSB0byBiZSB1cGRhdGVkIG9yIGRlbGV0ZWQgd2FzIGFscmVhZHkg
bW9kaWZpZWQgYnkgYW4gb3BlcmF0aW9uIHRyaWdnZXJlZCBieSB0aGUgY3VycmVudCBjb21tYW5k
IiksCisJCQkJCQkJIGVycmhpbnQoIkNvbnNpZGVyIHVzaW5nIGFuIEFGVEVSIHRyaWdnZXIgaW5z
dGVhZCBvZiBhIEJFRk9SRSB0cmlnZ2VyIHRvIHByb3BhZ2F0ZSBjaGFuZ2VzIHRvIG90aGVyIHJv
d3MuIikpKTsKKwogCQkJCWlmIChUcmFuc2FjdGlvbklkSXNDdXJyZW50VHJhbnNhY3Rpb25JZChj
b250ZXh0LT50bWZkLnhtYXgpKQogCQkJCQllcmVwb3J0KEVSUk9SLAogCQkJCQkJCShlcnJjb2Rl
KEVSUkNPREVfQ0FSRElOQUxJVFlfVklPTEFUSU9OKSwKQEAgLTMwMTAsNiArMzAzMSw3IEBAIGxt
ZXJnZV9tYXRjaGVkOgogCQkJCQkJCSBlcnJtc2coIiVzIGNvbW1hbmQgY2Fubm90IGFmZmVjdCBy
b3cgYSBzZWNvbmQgdGltZSIsCiAJCQkJCQkJCQkiTUVSR0UiKSwKIAkJCQkJCQkgZXJyaGludCgi
RW5zdXJlIHRoYXQgbm90IG1vcmUgdGhhbiBvbmUgc291cmNlIHJvdyBtYXRjaGVzIGFueSBvbmUg
dGFyZ2V0IHJvdy4iKSkpOworCiAJCQkJLyogVGhpcyBzaG91bGRuJ3QgaGFwcGVuICovCiAJCQkJ
ZWxvZyhFUlJPUiwgImF0dGVtcHRlZCB0byB1cGRhdGUgb3IgZGVsZXRlIGludmlzaWJsZSB0dXBs
ZSIpOwogCQkJCWJyZWFrOwpAQCAtMzExOCwxOSArMzE0MCwyNyBAQCBsbWVyZ2VfbWF0Y2hlZDoK
IAkJCQkJCQkvKgogCQkJCQkJCSAqIFRoaXMgY2FuIGJlIHJlYWNoZWQgd2hlbiBmb2xsb3dpbmcg
YW4gdXBkYXRlCiAJCQkJCQkJICogY2hhaW4gZnJvbSBhIHR1cGxlIHVwZGF0ZWQgYnkgYW5vdGhl
ciBzZXNzaW9uLAotCQkJCQkJCSAqIHJlYWNoaW5nIGEgdHVwbGUgdGhhdCB3YXMgYWxyZWFkeSB1
cGRhdGVkIGluCi0JCQkJCQkJICogdGhpcyB0cmFuc2FjdGlvbi4gSWYgcHJldmlvdXNseSBtb2Rp
ZmllZCBieQotCQkJCQkJCSAqIHRoaXMgY29tbWFuZCwgaWdub3JlIHRoZSByZWR1bmRhbnQgdXBk
YXRlLAotCQkJCQkJCSAqIG90aGVyd2lzZSBlcnJvciBvdXQuCi0JCQkJCQkJICoKLQkJCQkJCQkg
KiBTZWUgYWxzbyByZXNwb25zZSB0byBUTV9TZWxmTW9kaWZpZWQgaW4KLQkJCQkJCQkgKiBFeGVj
VXBkYXRlKCkuCisJCQkJCQkJICogcmVhY2hpbmcgYSB0dXBsZSB0aGF0IHdhcyBhbHJlYWR5IHVw
ZGF0ZWQgb3IKKwkJCQkJCQkgKiBkZWxldGVkIGJ5IHRoZSBjdXJyZW50IGNvbW1hbmQsIG9yIGJ5
IGEgbGF0ZXIKKwkJCQkJCQkgKiBjb21tYW5kIGluIHRoZSBjdXJyZW50IHRyYW5zYWN0aW9uLiBB
cyBhYm92ZSwKKwkJCQkJCQkgKiB0aGlzIHNob3VsZCBhbHdheXMgYmUgdHJlYXRlZCBhcyBhbiBl
cnJvci4KIAkJCQkJCQkgKi8KIAkJCQkJCQlpZiAoY29udGV4dC0+dG1mZC5jbWF4ICE9IGVzdGF0
ZS0+ZXNfb3V0cHV0X2NpZCkKIAkJCQkJCQkJZXJlcG9ydChFUlJPUiwKIAkJCQkJCQkJCQkoZXJy
Y29kZShFUlJDT0RFX1RSSUdHRVJFRF9EQVRBX0NIQU5HRV9WSU9MQVRJT04pLAogCQkJCQkJCQkJ
CSBlcnJtc2coInR1cGxlIHRvIGJlIHVwZGF0ZWQgb3IgZGVsZXRlZCB3YXMgYWxyZWFkeSBtb2Rp
ZmllZCBieSBhbiBvcGVyYXRpb24gdHJpZ2dlcmVkIGJ5IHRoZSBjdXJyZW50IGNvbW1hbmQiKSwK
IAkJCQkJCQkJCQkgZXJyaGludCgiQ29uc2lkZXIgdXNpbmcgYW4gQUZURVIgdHJpZ2dlciBpbnN0
ZWFkIG9mIGEgQkVGT1JFIHRyaWdnZXIgdG8gcHJvcGFnYXRlIGNoYW5nZXMgdG8gb3RoZXIgcm93
cy4iKSkpOworCisJCQkJCQkJaWYgKFRyYW5zYWN0aW9uSWRJc0N1cnJlbnRUcmFuc2FjdGlvbklk
KGNvbnRleHQtPnRtZmQueG1heCkpCisJCQkJCQkJCWVyZXBvcnQoRVJST1IsCisJCQkJCQkJCQkJ
KGVycmNvZGUoRVJSQ09ERV9DQVJESU5BTElUWV9WSU9MQVRJT04pLAorCQkJCQkJCQkvKiB0cmFu
c2xhdG9yOiAlcyBpcyBhIFNRTCBjb21tYW5kIG5hbWUgKi8KKwkJCQkJCQkJCQkgZXJybXNnKCIl
cyBjb21tYW5kIGNhbm5vdCBhZmZlY3Qgcm93IGEgc2Vjb25kIHRpbWUiLAorCQkJCQkJCQkJCQkJ
Ik1FUkdFIiksCisJCQkJCQkJCQkJIGVycmhpbnQoIkVuc3VyZSB0aGF0IG5vdCBtb3JlIHRoYW4g
b25lIHNvdXJjZSByb3cgbWF0Y2hlcyBhbnkgb25lIHRhcmdldCByb3cuIikpKTsKKworCQkJCQkJ
CS8qIFRoaXMgc2hvdWxkbid0IGhhcHBlbiAqLworCQkJCQkJCWVsb2coRVJST1IsICJhdHRlbXB0
ZWQgdG8gdXBkYXRlIG9yIGRlbGV0ZSBpbnZpc2libGUgdHVwbGUiKTsKIAkJCQkJCQlyZXR1cm4g
ZmFsc2U7CiAKIAkJCQkJCWRlZmF1bHQ6Cg==
--00000000000050f8f80612ea316a--