cleaning up a few CLOG-related things

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: cleaning up a few CLOG-related things
Date: 2021-01-25 16:56:13
Message-ID: CA+TgmoZYig9+AQodhF5sRXuKkJ=RgFDugLr3XX_dz_F-p=TwTg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I attach a series of proposed patches to slightly improve some minor
things related to the CLOG code.

0001 - Always call StartupCLOG() just after initializing
ShmemVariableCache. Right now, the hot_standby=off code path does this
at end of recovery, and the hot_standby=on code path does it at the
beginning of recovery. It's better to do this in only one place
because (a) it's simpler, (b) StartupCLOG() is trivial so trying to
postpone it in the hot_standby=off case has no value, and (c) it
allows for 0002 and therefore 0003, which make things even simpler.

0002 - In clog_redo(), don't set XactCtl->shared->latest_page_number.
The value that is being set here is actually the oldest page we're not
truncating, not the newest page that exists, so it's a bogus value
(except when we're truncating all but one page). The reason it's like
this now is to avoid having SimpleLruTruncate() see an uninitialized
value that might trip a sanity check, but after 0001 that won't be
possible, so we can just let the sanity check do its thing.

0003 - In TrimCLOG(), don't reset XactCtl->shared->latest_page_number.
After we stop doing 0002 we don't need to do this either, because the
only thing this can be doing for us is correcting the error introduced
by the code which 0002 removes. Relying on the results of replaying
(authoritative) CLOG/EXTEND records seems better than relying on our
(approximate) value of nextXid at end of recovery.

0004 - In StartupCLOG(), correct an off-by-one error. Currently, if
the nextXid is exactly a multiple of the number of CLOG entries that
fit on a page, then the value we compute for
XactCtl->shared->latest_page_number is higher than it should be by 1.
That's because nextXid represents the first XID that hasn't yet been
allocated, not the last one that gets allocated. So, the CLOG page
gets created when nextXid advances from the first value on the page to
the second value on the page, not when it advances from the last value
on the previous page to the first value on the current page.

Note that all of 0001-0003 result in a net removal of code. 0001 comes
out to more lines total because of the comment changes, but fewer
executable lines.

I don't plan to back-patch any of this because, AFAICS, an incorrect
value of XactCtl->shared->latest_page_number has no real consequences.
The SLRU code uses latest_page_number for just two purposes. First,
the latest page is never evicted; but that's just a question of
performance, not correctness, and the performance impact is small.
Second, the sanity check in SimpleLruTruncate() uses it. The present
code can make the value substantially inaccurate during recovery, but
only in a way that can make the sanity check pass rather than failing,
so it's not going to really bite anybody except perhaps if they have a
corrupted cluster where they would have liked the sanity check to
catch some problem. When not in recovery, the value can be off by at
most one. I am not sure whether there's a theoretical risk of this
making SimpleLruTruncate()'s sanity check fail when it should have
passed, but even if there is the chances must be extremely remote.

Some of the other SLRUs have similar issues as a result of
copy-and-paste work over the years. I plan to look at tidying that
stuff up, too. However, I wanted to post (and probably commit) these
patches first, partly to get some feedback, and also because all the
cases are a little different and I want to make sure to do a proper
analysis of each one.

Any review very much appreciated.

Thanks,

--
Robert Haas
EDB: http://www.enterprisedb.com

Attachment Content-Type Size
v1-0003-In-TrimCLOG-don-t-reset-XactCtl-shared-latest_pag.patch application/octet-stream 1.7 KB
v1-0002-In-clog_redo-don-t-set-XactCtl-shared-latest_page.patch application/octet-stream 1.8 KB
v1-0004-In-StartupCLOG-correct-an-off-by-one-error.patch application/octet-stream 2.2 KB
v1-0001-Move-StartupCLOG-calls-to-just-after-we-initializ.patch application/octet-stream 2.2 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2021-01-25 17:00:15 Re: Is Recovery actually paused?
Previous Message Dave Cramer 2021-01-25 16:29:10 Re: Error on failed COMMIT