pgsql: Change ThisTimeLineID from a global variable to a local variable
Change ThisTimeLineID from a global variable to a local variable.
StartupXLOG() still has ThisTimeLineID as a local variable, but the
remaining code in xlog.c now needs to the relevant TimeLineID by some
other means. Mostly, this means that we now pass it as a function
parameter to a bunch of functions where we didn't previously.
However, a few cases require special handling:
- In functions that might be called by outside callers who
wouldn't necessarily know what timeline to specify, we get
the timeline ID from shared memory. XLogCtl->ThisTimeLineID
can be used in most cases since recovery is known to have
completed by the time those functions are called. In
xlog_redo(), we can use XLogCtl->replayEndTLI.
- XLogFileClose() needs to know the TLI of the open logfile.
Do that with a new global variable openLogTLI. While
someone could argue that this is just trading one global
variable for another, the new one has a far more narrow
purposes and is referenced in just a few places.
- read_backup_label() now returns the TLI that it obtains
by parsing the backup_label file. Previously, ReadRecord()
could be called to parse the checkpoint record without
ThisTimeLineID having been initialized. Now, the timeline
is passed down, and I didn't want to pass an uninitialized
variable; this change lets us avoid that. The old coding
didn't seem to have any practical consequences that we need
to worry about, but this is cleaner.
- In BootstrapXLOG(), it's just a constant.
Patch by me, reviewed and tested by Michael Paquier, Amul Sul, and
Álvaro Herrera.
Discussion: /messages/by-id/CA+TgmobfAAqhfWa1kaFBBFvX+5CjM=7TE=n4r4Q1o2bjbGYBpA@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/4a92a1c3d1c361ffb031ed05bf65b801241d7cdd
Modified Files
--------------
src/backend/access/transam/xlog.c | 340 +++++++++++++++++++++-----------------
src/include/access/xlog.h | 2 +-
2 files changed, 192 insertions(+), 150 deletions(-)
Hi Robert,
On Fri, Nov 05, 2021 at 04:55:52PM +0000, Robert Haas wrote:
Change ThisTimeLineID from a global variable to a local variable.
StartupXLOG() still has ThisTimeLineID as a local variable, but the
remaining code in xlog.c now needs to the relevant TimeLineID by some
other means. Mostly, this means that we now pass it as a function
parameter to a bunch of functions where we didn't previously.
However, a few cases require special handling:
lapwing looks unhappy after this commit:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2021-11-05%2022%3A40%3A10
xlog.c: In function 'StartupXLOG':
xlog.c:7249:5: error: 'checkPointLoc' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
xlog.c:6686:5: note: 'checkPointLoc' was declared here
Thanks,
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Nov 05, 2021 at 04:55:52PM +0000, Robert Haas wrote:
Change ThisTimeLineID from a global variable to a local variable.
lapwing looks unhappy after this commit:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lapwing&dt=2021-11-05%2022%3A40%3A10
xlog.c: In function 'StartupXLOG':
xlog.c:7249:5: error: 'checkPointLoc' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
xlog.c:6686:5: note: 'checkPointLoc' was declared here
Yeah. I've seen some older compilers complain about that code before,
but now there are over a dozen buildfarm members warning about it.
It's clearly a false positive, since checkPointLoc does get set in
the read_backup_label()-failure-return code path. I think we've
seen a few other places where likely-to-be-inlined functions have
to initialize their result variables to prevent compiler warnings,
so I stuck an initialization step into read_backup_label in hopes
of silencing it.
regards, tom lane
On Sun, Nov 7, 2021 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah. I've seen some older compilers complain about that code before,
but now there are over a dozen buildfarm members warning about it.
It's clearly a false positive, since checkPointLoc does get set in
the read_backup_label()-failure-return code path. I think we've
seen a few other places where likely-to-be-inlined functions have
to initialize their result variables to prevent compiler warnings,
so I stuck an initialization step into read_backup_label in hopes
of silencing it.
That makes sense, and sorry for not noticing this discussion sooner.
But, I'm a bit confused about what's going on here. Why did we start
getting this complaint only now? AFAICS I didn't change anything about
how checkPointLoc is handled. It's an XLogRecPtr, not a TLI, and all
the stuff I changed has to do with TLI handling. I think, anyway.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Sun, Nov 7, 2021 at 12:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah. I've seen some older compilers complain about that code before,
but now there are over a dozen buildfarm members warning about it.
It's clearly a false positive, since checkPointLoc does get set in
the read_backup_label()-failure-return code path. I think we've
seen a few other places where likely-to-be-inlined functions have
to initialize their result variables to prevent compiler warnings,
so I stuck an initialization step into read_backup_label in hopes
of silencing it.
That makes sense, and sorry for not noticing this discussion sooner.
But, I'm a bit confused about what's going on here. Why did we start
getting this complaint only now? AFAICS I didn't change anything about
how checkPointLoc is handled.
I think it's pretty clearly a compiler bug. The triggering conditions
are obscure, but they seem to have to do with inlining a function that
is supposed to initialize some variables but skips doing so in some
code paths for which "it shouldn't matter". You'd think that having
inlined, the compiler could see that all live code paths initialize
the variable ... but sometimes it doesn't see that.
Based on this data point, we can speculate that the number of such
variables matters, too.
Also, as I said, I've seen compilers warn about checkPointLoc before.
prairiedog's admittedly-ancient gcc has been doing so for years.
regards, tom lane
On Sun, Nov 7, 2021 at 4:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Based on this data point, we can speculate that the number of such
variables matters, too.
Yeah, apparently. Well, at least I don't feel bad about not guessing
that that would happen. That's pretty magical.
--
Robert Haas
EDB: http://www.enterprisedb.com