Обсуждение: 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