Re: Asynchronous MergeAppend

Hi.

Matheus Alcantara писал(а) 2025-12-17 23:01:
> Thanks for the new version. I don't have other comments on the current
> state of the patch. It seems to working as expected and we have
> performance improvements that I think that it make it worthwhile.
> 
> I have just a small comment on 0002:
> 
> +    noccurred = WaitEventSetWait(node->ms_eventset, -1 /* no timeout */ , 
> occurred_event,
> +                                 nevents, WAIT_EVENT_APPEND_READY);
> 
> Should we use the same WAIT_EVENT_APPEND_READY or create a new wait
> event for merge append?

I'm not sure that new wait event is needed - for observability I think 
it's not critical
to distinguish Append and MergeAppend when they waited for foreign 
scans.  But also it's perhaps
doesn't do any harm to record specific wait event.

> 
> I've spent some time thinking about how we could remove some parts of
> the duplicated code that you've previously mention. I think that we
> could try to create something like we already have for relation scan
> operations, that we have execScan.c that is used for example by
> nodeSeqScan.c and nodeIndexScan.c. The attached patch 0003 is a draft
> that I attempt to implement this idea. The 0001 and 0002 remains the
> same as the previous version. The 0003 was build on top of these.
> 
> I've created Appender and AppenderState types that are used by
> Append/MergeAppend and AppendState/MergeAppendState respectively (I
> should have think in a better name for these base type, ideas are
> welcome). The execAppend.c was created to have the functions that can 
> be
> reused by Append and MergeAppend execution. I've tried to remove
> duplicated code blocks that was almost the same and that didn't require
> much refactoring.

Overall I like new Appender node. Splitting code in this way really 
helps to avoid code duplication.
However, some similar code is still needed, because logic of getting new 
tuples is different.

Some minor issues I've noticed.
1) ExecReScanAppender()  sets node->needrequest to NULL. 
ExecReScanAppend() calls bms_free(node->as.needrequest) immediately 
after this. The same is true for ExecReScanMergeAppend(). We should move 
it to ExecReScanAppender().

2) In src/backend/executor/execAppend.c:
planstates are named  as mergeplans in ExecEndAppender(), perhaps, 
appendplans or subplans are better names.

ExecInitAppender() could use palloc_array() to allocate appendplanstates 
- as ExecInitMergeAppend().


> 
> I think that there still more opportunities to remove similar code
> blocks that for example are on ExecMergeAppendAsyncGetNext and
> ExecAppendAsyncGetNext but it require refactoring.
> 
> Thoughts?
> 
> --
> Matheus Alcantara
> EDB: http://www.enterprisedb.com

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional