On Mon, Jan 15, 2024 at 4:03 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> It doesn't pass the number of LP_DEAD items back to the caller. It
> passes a boolean.
Oops.
> Perhaps it isn't important, but I find this wording confusing. You
> mention lazy_scan_prune() and then mention that "the whole test is in
> one place instead of spread out" -- which kind of makes it sound like
> you are consolidating FSM updates for both the lazy_scan_noprune() and
> lazy_scan_prune() cases. Perhaps simply flipping the order of the "since
> the caller" and "moreover, this way" conjunctions would solve it. I
> defer to your judgment.
I rewrote the commit message a bit. See what you think of this version.
> tests if lpdead_items > 0. It is more the inconsistency that bothered me
> than the fact that lpdead_items is signed.
Changed.
> > @@ -2159,6 +2156,9 @@ lazy_scan_noprune(LVRelState *vacrel,
> > if (hastup)
> > vacrel->nonempty_pages = blkno + 1;
> >
> > + /* Did we find LP_DEAD items? */
> > + *has_lpdead_items = (lpdead_items > 0);
>
> I would drop this comment. The code doesn't really need it, and the
> reason we care if there are LP_DEAD items is not because they are dead
> but because we want to know if we'll touch this page again. You don't
> need to rehash all that here, so I think omitting the comment is enough.
I want to keep the comment. I guess it's a pet peeve of mine, but I
hate it when people do this:
/* some comment */
some_code();
some_more_code();
/* some other comment */
even_more_code();
IMV, this makes it unclear whether /* some comment */ is describing
both some_code() and some_more_code(), or just the former. To be fair,
there is often no practical confusion, because if the comment is good
and the code is nothing too complicated then you understand which way
the author meant it. But sometimes the comment is bad or out of date
and sometimes the code is difficult to understand and then you're left
scratching your head as to what the author meant. I prefer to insert a
comment above some_more_code() in such cases, even if it's a bit
perfunctory. I think it makes the code easier to read.
--
Robert Haas
EDB: http://www.enterprisedb.com