Thread: 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: https://postgr.es/m/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(-)
Re: pgsql: Change ThisTimeLineID from a global variable to a local variable
From
Michael Paquier
Date:
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
Attachment
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