On Wed, Aug 2, 2023 at 11:21 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapil...@gmail.com> wrote:
> > >
> > > On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.m...@gmail.com>
> > > wrote:
> > > >
> > > > While reading the code, I realized that the following code comments
> > > > might not be accurate:
> > > >
> > > > /*
> > > > * Pick the largest transaction (or subtransaction) and evict
> > > > it from
> > > > * memory by streaming, if possible. Otherwise, spill to disk.
> > > > */
> > > > if (ReorderBufferCanStartStreaming(rb) &&
> > > > (txn = ReorderBufferLargestStreamableTopTXN(rb)) != NULL)
> > > > {
> > > > /* we know there has to be one, because the size is not
> > > > zero */
> > > > Assert(txn && rbtxn_is_toptxn(txn));
> > > > Assert(txn->total_size > 0);
> > > > Assert(rb->size >= txn->total_size);
> > > >
> > > > ReorderBufferStreamTXN(rb, txn);
> > > > }
> > > >
> > > > AFAICS since ReorderBufferLargestStreamableTopTXN() returns only
> > > > top-level transactions, the comment above the if statement is not
> > > > right. It would not pick a subtransaction.
> > > >
> > >
> > > I think the subtransaction case is for the spill-to-disk case as both
> > > cases are explained in the same comment.
> > >
> > > > Also, I'm not sure that the second comment "we know there has to be
> > > > one, because the size is not zero" is right since there might not be
> > > > top-transactions that are streamable.
> > > >
> > >
> > > I think this comment is probably referring to asserts related to the
> > > size similar to spill to disk case.
> > >
> > > How about if we just remove (or subtransaction) from the following
> > > comment: "Pick the largest transaction (or subtransaction) and evict
> > > it from memory by streaming, if possible. Otherwise, spill to disk."?
> > > Then by referring to streaming/spill-to-disk cases, one can understand
> > > in which cases only top-level xacts are involved and in which cases
> > > both are involved.
> >
> > Sounds good. I've updated the patch accordingly.
> >
>
> LGTM.
Thank you for reviewing the patch! Pushed.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
