Обсуждение: Inaccurate comments in ReorderBufferCheckMemoryLimit()

Hi all,

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.

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 is why we say
"Otherwise, spill to disk".

I've attached a patch to fix these comments. Feedback is very welcome.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@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.

--
With Regards,
Amit Kapila.

On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@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.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@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.

--
With Regards,
Amit Kapila.

On Wed, Aug 2, 2023 at 11:21 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 1, 2023 at 2:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Jul 31, 2023 at 8:46 PM Masahiko Sawada <sawada.mshk@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