From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Inaccurate comments in ReorderBufferCheckMemoryLimit() |
Date: | 2023-08-01 08:36:02 |
Message-ID: | CAD21AoDbBC2sGLLbDD_QXXFcMg2uPKobM8PVt0xdZBG8D-dRvQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Aug 1, 2023 at 11:33 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.
Sounds good. I've updated the patch accordingly.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Fix-ReorderBufferCheckMemoryLimit-comment.patch | application/octet-stream | 1.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Richard Guo | 2023-08-01 08:44:27 | Oversight in reparameterize_path_by_child leading to executor crash |
Previous Message | Daniel Gustafsson | 2023-08-01 08:13:32 | Re: [PATCH] Tracking statements entry timestamp in pg_stat_statements |