Re: Inaccurate comments in ReorderBufferCheckMemoryLimit()

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