Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Mon, 06 Apr 2026 20:48:05 -0300
Message-Id: <DHMH23M7UOFS.12W6PUDI1I3NH@gmail.com>
Cc: "Alexander Pyhalov" <a.pyhalov@postgrespro.ru>, "Pgsql Hackers"
 <pgsql-hackers@postgresql.org>
Subject: Re: Asynchronous MergeAppend
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
To: "Alexander Korotkov" <aekorotkov@gmail.com>
References: <59be194c5a409fb9fc9f2031581b8a44@postgrespro.ru>
 <2fb1d9923b6995492e7b163e6cb95402@postgrespro.ru>
 <DE662JHGRO2O.3KJBBG2R3IT17@gmail.com>
 <efbfbcf00b6b790e8f80c13a83417a23@postgrespro.ru>
 <CAFY6G8d3Yvxa_kRQA24BsJhwqfmSCv1ujiv_7b6g5isf-ZTs_Q@mail.gmail.com>
 <b546d8bd5e2218f4f917fda4c93ead21@postgrespro.ru>
 <DED05POJZS2W.2EZ60AOBMDDAE@gmail.com>
 <bcdefce7e3db9566d0619ad2eed2a299@postgrespro.ru>
 <DF0RCOYB19RC.35BW2FHPO4N4S@gmail.com>
 <a16423ca928cde3650bbc219982a5ffc@postgrespro.ru>
 <DF28LV8UTUOO.71JUZDH1N8F9@gmail.com>
 <d5c0bfca6ee15d62df1ba72e540b4ddf@postgrespro.ru>
 <DFAQTF8P9AU0.2ZELZFXOCD0XL@gmail.com>
 <c264abe661f72d6627f987fc9aa4e456@postgrespro.ru>
 <DFBN6K9K9IUG.23WZ7OBL8DRAQ@gmail.com>
 <c0d55d3f68bfd5650fe3143d3a31c84c@postgrespro.ru>
 <782a968c8e01ec6db3b2da2120adf73b@postgrespro.ru>
 <554a73f7ffea8b22b3f81a4804b5fc34@postgrespro.ru>
 <e13717179328abed3e189f4ca1e63087@postgrespro.ru>
 <CAPpHfdu+3Eud0CBpdFT+osWiT=e=zOQUtBsx8Z5okKrqhgVAJg@mail.gmail.com>
 <42a9a941-e768-4fd8-8067-12958c5a1d70@gmail.com>
 <CAPpHfdsO8zYpDW==D6T5N0cJ+AzK7a_OyXJoYU1kFi=xZFTLuQ@mail.gmail.com>
In-Reply-To: 
 <CAPpHfdsO8zYpDW==D6T5N0cJ+AzK7a_OyXJoYU1kFi=xZFTLuQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DHMH23M7UOFS.12W6PUDI1I3NH%40gmail.com>
Precedence: bulk

On Sat Apr 4, 2026 at 11:24 PM -03, Alexander Korotkov wrote:
> I made more work on the patchset.
>
> Patch #1 now considers IncrementalSort as exclusion alongside with
> Sort.  Exclusion check is now on the top of the switch().
> Patch #2 is split into 3 patches: common structures, common sync
> append logic, and common async append logic.
> New structs are now named AppendBase/AppendBaseState, corresponding
> fields are "ab" and "as".
>
> Most importantly I noted that this patchset actually only makes
> initial heap filling asynchronous.  The steady work after that is
> still syncnronous.  Even that it used async infrastructure, it fetched
> tuples from children subplans one-by-one: effectively synchronous but
> paying for asynchronous infrastructure.  I think even with this
> limitation, this patchset is valuable: the startup cost for children
> foreignscans can be high.  But this understanding allowed me to
> significantly simplify the main patch including:
> 1) After initial heap filling, use ExecProcNode() to fetch from children =
plans.
> 2) Remove ms_has_asyncresults entirely. Async responses store directly
> into ms_slots[] (the existing heap slot array), which serves as both
> the merge state and the "result arrived" indicator via TupIsNull().
> 3) Removed needrequest usage from MergeAppend. Since MergeAppend only
> fires initial requests (via ExecAppendBaseAsyncBegin()) and never
> sends follow-up requests, needrequest tracking is unnecessary.
> ExecMergeAppendAsyncRequest() was eliminated entirely.
> 4)  ExecMergeAppendAsyncGetNext() reduced to a simple wait loop:
> 5)  asyncresults allocation reduced back to nasyncplans.  MergeAppend
> doesn't use it (stores in ms_slots), and Append only needs nasyncplans
> entries for its stack.
>
> Additionally, I made the following changes.
> 1) WAIT_EVENT_MERGE_APPEND_READY wait event instead of extending
> WAIT_EVENT_APPEND_READY.  That should be less confusing for monitoring
> purposes.
> 2) More tests: error handling with broken partition, plan-time
> partition pruning, and run-time partition pruning tests for async
> MergeAppend.
>

Thanks for v17, the split of 0002 into multiple patches seems much
better. Overall I agree with the changes on v17 compared with v16, the
removal of ms_has_asyncresults makes the code better to read and follow.
The separate WAIT_EVENT_MERGE_APPEND_READY for better monitoring is also
good, I've tested some long running queries and I've find the event on
pg_stat_activity. The steady work changes also looks good.

One minor issue on 0002:

+	bool		valid_subplans_identified;	/* is valid_asyncplans valid? */
+	Bitmapset  *valid_subplans;
+	Bitmapset  *valid_asyncplans;	/* valid asynchronous plans indexes */

Should be /* is valid_subplans valid? */

-----

Minor comment on 0005:

+static void
+ExecMergeAppendAsyncGetNext(MergeAppendState *node, int mplan)
+{
+	/*
+	 * All initial async requests were fired by ExecAppendBaseAsyncBegin.

Wondering if we should reference ExecMergeAppendAsyncBegin() instead of
ExecAppendBaseAsyncBegin() since this is on nodeMergeAppend, what do you
think?

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