On Mon, Apr 1, 2024 at 11:26 AM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Fri, Mar 29, 2024 at 7:37 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Mar 29, 2024 at 12:13 PM Masahiko Sawada <sawada.m...@gmail.com> > > wrote: > > > > > > On Fri, Mar 29, 2024 at 2:09 PM vignesh C <vignes...@gmail.com> wrote: > > > > > > > > On Tue, 26 Mar 2024 at 10:05, Masahiko Sawada <sawada.m...@gmail.com> > > > > wrote: > > > > > > > > > > On Thu, Mar 14, 2024 at 12:02 PM Masahiko Sawada > > > > > <sawada.m...@gmail.com> wrote: > > > > > > > > > > > > > > > > > > I've attached new version patches. > > > > > > > > > > Since the previous patch conflicts with the current HEAD, I've > > > > > attached the rebased patches. > > > > > > > > Thanks for the updated patch. > > > > One comment: > > > > I felt we can mention the improvement where we update memory > > > > accounting info at transaction level instead of per change level which > > > > is done in ReorderBufferCleanupTXN, ReorderBufferTruncateTXN, and > > > > ReorderBufferSerializeTXN also in the commit message: > > > > > > Agreed. > > > > > > I think the patch is in good shape. I'll push the patch with the > > > suggestion next week, barring any objections. > > > > > > > Few minor comments: > > 1. > > @@ -3636,6 +3801,8 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb) > > Assert(txn->nentries_mem == 0); > > } > > > > + ReorderBufferMaybeResetMaxHeap(rb); > > + > > > > Can we write a comment about why this reset is required here? > > Otherwise, the reason is not apparent. > > Yes, added. > > > > > 2. > > Although using max-heap to select the largest > > + * transaction is effective when there are many transactions being decoded, > > + * there is generally no need to use it as long as all transactions being > > + * decoded are top-level transactions. Therefore, we use MaxConnections as > > the > > + * threshold so we can prevent switching to the state unless we use > > + * subtransactions. > > + */ > > +#define MAX_HEAP_TXN_COUNT_THRESHOLD MaxConnections > > > > Isn't using max-heap equally effective in finding the largest > > transaction whether there are top-level or top-level plus > > subtransactions? This comment indicates it is only effective when > > there are subtransactions. > > You're right. Updated the comment. > > I've attached the updated patches. >
While reviewing the patches, I realized the comment of binearyheap_allocate() should also be updated. So I've attached the new patches. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v12-0001-Make-binaryheap-enlargeable.patch
Description: Binary data
v12-0002-Add-functions-to-binaryheap-for-efficient-key-re.patch
Description: Binary data
v12-0003-Improve-eviction-algorithm-in-Reorderbuffer-usin.patch
Description: Binary data