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. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v2-0001-Fix-ReorderBufferCheckMemoryLimit-comment.patch
Description: Binary data