Content-Type: multipart/mixed;
 boundary=c76db600636f81f156e46d51d5ed0158382c047f16d5e7a69c2125e4a5b5
Mime-Version: 1.0
Date: Wed, 19 Nov 2025 18:51:35 -0300
Message-Id: <DED05POJZS2W.2EZ60AOBMDDAE@gmail.com>
Cc: "Alena Rybakina" <a.rybakina@postgrespro.ru>, "Pgsql Hackers"
 <pgsql-hackers@postgresql.org>
Subject: Re: Asynchronous MergeAppend
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
To: "Alexander Pyhalov" <a.pyhalov@postgrespro.ru>, "Matheus Alcantara"
 <matheusssilv97@gmail.com>
References: <59be194c5a409fb9fc9f2031581b8a44@postgrespro.ru>
 <764dd8b8-6374-4f5a-aac7-d8e3f6ebe5fd@postgrespro.ru>
 <159b591411bb2c81332018927acbd509@postgrespro.ru>
 <DDZ2ULUYDQJ4.MXMP02V4GIG@gmail.com>
 <2fb1d9923b6995492e7b163e6cb95402@postgrespro.ru>
 <DE662JHGRO2O.3KJBBG2R3IT17@gmail.com>
 <efbfbcf00b6b790e8f80c13a83417a23@postgrespro.ru>
 <CAFY6G8d3Yvxa_kRQA24BsJhwqfmSCv1ujiv_7b6g5isf-ZTs_Q@mail.gmail.com>
 <b546d8bd5e2218f4f917fda4c93ead21@postgrespro.ru>
In-Reply-To: <b546d8bd5e2218f4f917fda4c93ead21@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/DED05POJZS2W.2EZ60AOBMDDAE%40gmail.com>
Precedence: bulk

--c76db600636f81f156e46d51d5ed0158382c047f16d5e7a69c2125e4a5b5
Content-Type: multipart/alternative;
 boundary=b90ee82cf39a4ef1ef0f663a5a6264756f3d9b1cac3144b3ea850b7e806a

--b90ee82cf39a4ef1ef0f663a5a6264756f3d9b1cac3144b3ea850b7e806a
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Content-Type: text/plain; charset=UTF-8

On Tue Nov 18, 2025 at 4:14 AM -03, Alexander Pyhalov wrote:
> Updated the first patch.
>
Thanks for the new version. Some new comments.

v7-0002-MergeAppend-should-support-Async-Foreign-Scan-subpla.patch:

1. Should be "nasyncplans" instead of "nplans"? ExecInitAppend use
"nasyncplans" to allocate the as_asyncresults array.

+		mergestate->ms_asyncresults =3D (TupleTableSlot **)
+			palloc0(nplans * sizeof(TupleTableSlot *));
+

2. I think that this comment should be updated. IIUC ms_valid_subplans
may not be all subplans because classify_matching_subplans() may move
async ones to ms_valid_asyncplans. Is that right?

/*
 * If we've yet to determine the valid subplans then do so now.  If
 * run-time pruning is disabled then the valid subplans will always be
 * set to all subplans.
 */

3. This code comment should also mention the Assert(!bms_is_member(...));?

+	/* The result should be a TupleTableSlot or NULL. */
+	Assert(slot =3D=3D NULL || IsA(slot, TupleTableSlot));
+	Assert(!bms_is_member(areq->request_index, node->ms_has_asyncresults));

4. It's worth include bms_num_members(node->ms_needrequest) <=3D 0 check
on ExecMergeAppendAsyncRequest() as an early return? IIUC it would avoid
the bms_is_member(), bms_next_member() and bms_is_empty(needrequest)
calls.

ExecMergeAppendAsyncRequest(MergeAppendState *node, int mplan)
        Bitmapset  *needrequest;
        int                     i;

+       if (bms_num_members(node->ms_needrequest) <=3D 0)
+               return false;
+

I'm attaching a diff with some cosmetic changes of indentation and
comments. Feel free to include on the patch or not.

--
Matheus Alcantara
EDB: http://www.enterprisedb.com


--b90ee82cf39a4ef1ef0f663a5a6264756f3d9b1cac3144b3ea850b7e806a--

--c76db600636f81f156e46d51d5ed0158382c047f16d5e7a69c2125e4a5b5
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=diff.txt
Content-Type: text/plain; charset=utf-8; name=diff.txt

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVBcHBlbmQuYyBiL3NyYy9iYWNr
ZW5kL2V4ZWN1dG9yL25vZGVBcHBlbmQuYwppbmRleCBjODllMmQyNzg3Zi4uYTJlZDVmNzFhMzUg
MTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVBcHBlbmQuYworKysgYi9zcmMv
YmFja2VuZC9leGVjdXRvci9ub2RlQXBwZW5kLmMKQEAgLTExOTgsNiArMTE5OCw5IEBAIGNsYXNz
aWZ5X21hdGNoaW5nX3N1YnBsYW5zKEFwcGVuZFN0YXRlICpub2RlKQogCX0KIAogCS8qIE5vIHZh
bGlkIGFzeW5jIHN1YnBsYW5zIGlkZW50aWZpZWQuICovCi0JaWYgKCFjbGFzc2lmeV9tYXRjaGlu
Z19zdWJwbGFuc19jb21tb24oJm5vZGUtPmFzX3ZhbGlkX3N1YnBsYW5zLCBub2RlLT5hc19hc3lu
Y3BsYW5zLCAmbm9kZS0+YXNfdmFsaWRfYXN5bmNwbGFucykpCisJaWYgKCFjbGFzc2lmeV9tYXRj
aGluZ19zdWJwbGFuc19jb21tb24oCisJCQkJCQkJCQkJICAgJm5vZGUtPmFzX3ZhbGlkX3N1YnBs
YW5zLAorCQkJCQkJCQkJCSAgIG5vZGUtPmFzX2FzeW5jcGxhbnMsCisJCQkJCQkJCQkJICAgJm5v
ZGUtPmFzX3ZhbGlkX2FzeW5jcGxhbnMpKQogCQlub2RlLT5hc19uYXN5bmNyZW1haW4gPSAwOwog
fQpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvZXhlY3V0b3Ivbm9kZU1lcmdlQXBwZW5kLmMgYi9z
cmMvYmFja2VuZC9leGVjdXRvci9ub2RlTWVyZ2VBcHBlbmQuYwppbmRleCA3ZGI0MWZiZjQwZi4u
ZmViMjVhODEzYjEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVNZXJnZUFw
cGVuZC5jCisrKyBiL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVNZXJnZUFwcGVuZC5jCkBAIC01
MzMsNyArNTMzLDEwIEBAIGNsYXNzaWZ5X21hdGNoaW5nX3N1YnBsYW5zKE1lcmdlQXBwZW5kU3Rh
dGUgKm5vZGUpCiAJfQogCiAJLyogTm8gdmFsaWQgYXN5bmMgc3VicGxhbnMgaWRlbnRpZmllZC4g
Ki8KLQlpZiAoIWNsYXNzaWZ5X21hdGNoaW5nX3N1YnBsYW5zX2NvbW1vbigmbm9kZS0+bXNfdmFs
aWRfc3VicGxhbnMsIG5vZGUtPm1zX2FzeW5jcGxhbnMsICZub2RlLT5tc192YWxpZF9hc3luY3Bs
YW5zKSkKKwlpZiAoIWNsYXNzaWZ5X21hdGNoaW5nX3N1YnBsYW5zX2NvbW1vbigKKwkJCQkJCQkJ
CQkgICAmbm9kZS0+bXNfdmFsaWRfc3VicGxhbnMsCisJCQkJCQkJCQkJICAgbm9kZS0+bXNfYXN5
bmNwbGFucywKKwkJCQkJCQkJCQkgICAmbm9kZS0+bXNfdmFsaWRfYXN5bmNwbGFucykpCiAJCW5v
ZGUtPm1zX2FzeW5jcmVtYWluID0gTlVMTDsKIH0KIApAQCAtNzIxLDExICs3MjQsMTQgQEAgRXhl
Y0FzeW5jTWVyZ2VBcHBlbmRSZXNwb25zZShBc3luY1JlcXVlc3QgKmFyZXEpCiAJewogCQkvKiBU
aGUgZW5kaW5nIHN1YnBsYW4gd291bGRuJ3QgaGF2ZSBiZWVuIHBlbmRpbmcgZm9yIGEgY2FsbGJh
Y2suICovCiAJCUFzc2VydCghYXJlcS0+Y2FsbGJhY2tfcGVuZGluZyk7Ci0JCW5vZGUtPm1zX2Fz
eW5jcmVtYWluID0gYm1zX2RlbF9tZW1iZXIobm9kZS0+bXNfYXN5bmNyZW1haW4sIGFyZXEtPnJl
cXVlc3RfaW5kZXgpOworCQlub2RlLT5tc19hc3luY3JlbWFpbiA9IGJtc19kZWxfbWVtYmVyKG5v
ZGUtPm1zX2FzeW5jcmVtYWluLAorCQkJCQkJCQkJCQkgIGFyZXEtPnJlcXVlc3RfaW5kZXgpOwog
CQlyZXR1cm47CiAJfQogCi0Jbm9kZS0+bXNfaGFzX2FzeW5jcmVzdWx0cyA9IGJtc19hZGRfbWVt
YmVyKG5vZGUtPm1zX2hhc19hc3luY3Jlc3VsdHMsIGFyZXEtPnJlcXVlc3RfaW5kZXgpOworCS8q
IE1hcmsgdGhhdCB0aGUgYXN5bmMgcmVxdWVzdCBoYXMgYSByZXN1bHQgKi8KKwlub2RlLT5tc19o
YXNfYXN5bmNyZXN1bHRzID0gYm1zX2FkZF9tZW1iZXIobm9kZS0+bXNfaGFzX2FzeW5jcmVzdWx0
cywKKwkJCQkJCQkJCQkJICAgYXJlcS0+cmVxdWVzdF9pbmRleCk7CiAJLyogU2F2ZSByZXN1bHQg
c28gd2UgY2FuIHJldHVybiBpdC4gKi8KIAlub2RlLT5tc19hc3luY3Jlc3VsdHNbYXJlcS0+cmVx
dWVzdF9pbmRleF0gPSBzbG90OwogCmRpZmYgLS1naXQgYS9zcmMvaW5jbHVkZS9ub2Rlcy9leGVj
bm9kZXMuaCBiL3NyYy9pbmNsdWRlL25vZGVzL2V4ZWNub2Rlcy5oCmluZGV4IDJiZWY1NDU1MGEz
Li4xZGM3ZTllNjE0NSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvbm9kZXMvZXhlY25vZGVzLmgK
KysrIGIvc3JjL2luY2x1ZGUvbm9kZXMvZXhlY25vZGVzLmgKQEAgLTE1ODUsNyArMTU4NSw5IEBA
IEdldE5lZWRSZXF1ZXN0KFBsYW5TdGF0ZSAqcHMpCiAKIC8qIENvbW1vbiBwYXJ0IG9mIGNsYXNz
aWZ5X21hdGNoaW5nX3N1YnBsYW5zKCkgZm9yIEFwcGVuZCBhbmQgTWVyZ2VBcHBlbmQgKi8KIHN0
YXRpYyBpbmxpbmUgYm9vbAotY2xhc3NpZnlfbWF0Y2hpbmdfc3VicGxhbnNfY29tbW9uKEJpdG1h
cHNldCAqKnZhbGlkX3N1YnBsYW5zLCBCaXRtYXBzZXQgKmFzeW5jcGxhbnMsIEJpdG1hcHNldCAq
KnZhbGlkX2FzeW5jcGxhbnMpCitjbGFzc2lmeV9tYXRjaGluZ19zdWJwbGFuc19jb21tb24oQml0
bWFwc2V0ICoqdmFsaWRfc3VicGxhbnMsCisJCQkJCQkJCSAgQml0bWFwc2V0ICphc3luY3BsYW5z
LAorCQkJCQkJCQkgIEJpdG1hcHNldCAqKnZhbGlkX2FzeW5jcGxhbnMpCiB7CiAJQXNzZXJ0KCp2
YWxpZF9hc3luY3BsYW5zID09IE5VTEwpOwogCg==
--c76db600636f81f156e46d51d5ed0158382c047f16d5e7a69c2125e4a5b5--