Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 11 Nov 2025 18:00:53 -0300
Message-Id: <DE662JHGRO2O.3KJBBG2R3IT17@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>
In-Reply-To: <2fb1d9923b6995492e7b163e6cb95402@postgrespro.ru>
Archived-At: 
 <https://www.postgresql.org/message-id/DE662JHGRO2O.3KJBBG2R3IT17%40gmail.com>
Precedence: bulk

On Wed Nov 5, 2025 at 3:30 AM -03, Alexander Pyhalov wrote:
>> So I'm +1 for the idea. I know it's been while since the last patch,
>> and unfortunully it hasn't received reviews since then. Do you still
>> plan to work on it? I still need to take a look on the code to see if
>> I can help with some comments.
>
> I'm still interested in working on this patch, but it didn't get any=20
> review (besides internal one). I suppose, Append and MergeAppend nodes=20
> need some unification, for example, ExecAppendAsyncEventWait and=20
> ExecMergeAppendAsyncEventWait looks the same, both=20
> classify_matching_subplans() versions are suspiciously similar. But=20
> honestly, patch needs thorough review.
>
Here are some comments on my first look at the patches. I still don't
have too much experience with the executor code but I hope that I can
help with something.

v5-0001-mark_async_capable-subpath-should-match-subplan.patch

I don't have to much comments on this, perhaps we could have a commit
message explaining the reason behind the change.=20

----

v5-0002-MergeAppend-should-support-Async-Foreign-Scan-sub.patch

The AppendState struct has the "as_syncdone", this field is not needed
on MergeAppendState?

----
Regarding the duplicated code on classify_matching_subplans I think that
we can have a more generic function that operates over function
parameters, something like this:

/*
 * Classify valid subplans into sync and async groups.
 *
 * It calculates the intersection of *valid_subplans and *asyncplans,
 * stores the result in *valid_asyncplans, and removes those members
 * from *valid_subplans (leaving only sync plans).
 *
 * Returns true if valid async plans were found, false otherwise.
 */
static bool
classify_subplans_internal(Bitmapset **valid_subplans,
                       Bitmapset *asyncplans,
                       Bitmapset **valid_asyncplans);

----
The GetValidAsyncplans() is not being used

----
We have some reduction of code coverage on nodeMergeAppend.c. The
significant blocks are on ExecMergeAppendAsyncBegin():
+	/* If we've yet to determine the valid subplans then do so now. */
+	if (!node->ms_valid_subplans_identified)
+	{
+		node->ms_valid_subplans =3D
+			ExecFindMatchingSubPlans(node->ms_prune_state, false, NULL);
+		node->ms_valid_subplans_identified =3D true;
+
+		classify_matching_subplans(node);
+	}

And there are some blocks on ExecReScanMergeAppend(). It's worth adding
a test case for then? I'm not sure how hard would be to write a
regression test that cover these blocks.

----
I agree that duplicated code is not good but it seems to me that we
already have some code on nodeMergeAppend.c borrowed from nodeAppend.c
even without you patch, for example the ExecInitMergeAppend(),
ExecReScanMergeAppend() and partially ExecMergeAppend().

Although nodeMergeAppend.c and nodeAppend.c have similar functions ,
some difference exists and I'm wondering if we should wait for the rule
of three [1] to refactor these duplicated code?

[1] https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

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