Thread: Unnecessary static variable?
While reading XLogPageRead() I was surprised that readLen variable is set but
not used in the read() call. Then I realized that it's declared static
although no other function uses it. Maybe it was used earlier to exit early if
sufficient amount of data was already read? I think this case is now handled
by the calling function xlogreader.c:ReadPageInternal().
I suggest to make the variable local:
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
new file mode 100644
index e42b828..c3267f5
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*************** static XLogSegNo openLogSegNo = 0;
*** 776,792 ****
static uint32 openLogOff = 0;
/*
! * These variables are used similarly to the ones above, but for reading
! * the XLOG. Note, however, that readOff generally represents the offset
! * of the page just read, not the seek position of the FD itself, which
! * will be just past that page. readLen indicates how much of the current
! * page has been read into readBuf, and readSource indicates where we got
! * the currently open file from.
*/
static int readFile = -1;
static XLogSegNo readSegNo = 0;
static uint32 readOff = 0;
- static uint32 readLen = 0;
static XLogSource readSource = 0; /* XLOG_FROM_* code */
/*
--- 776,790 ----
static uint32 openLogOff = 0;
/*
! * These variables are used similarly to the ones above, but for reading the
! * XLOG. Note, however, that readOff generally represents the offset of the
! * page just read, not the seek position of the FD itself, which will be just
! * past that page. readSource indicates where we got the currently open file
! * from.
*/
static int readFile = -1;
static XLogSegNo readSegNo = 0;
static uint32 readOff = 0;
static XLogSource readSource = 0; /* XLOG_FROM_* code */
/*
*************** XLogPageRead(XLogReaderState *xlogreader
*** 11556,11561 ****
--- 11554,11560 ----
(XLogPageReadPrivate *) xlogreader->private_data;
int emode = private->emode;
uint32 targetPageOff;
+ uint32 readLen;
XLogSegNo targetSegNo PG_USED_FOR_ASSERTS_ONLY;
XLByteToSeg(targetPagePtr, targetSegNo, wal_segment_size);
*************** retry:
*** 11603,11609 ****
if (readFile >= 0)
close(readFile);
readFile = -1;
- readLen = 0;
readSource = 0;
return -1;
--- 11602,11607 ----
*************** retry:
*** 11618,11626 ****
/*
* If the current segment is being streamed from master, calculate how
! * much of the current page we have received already. We know the
! * requested record has been received, but this is for the benefit of
! * future calls, to allow quick exit at the top of this function.
*/
if (readSource == XLOG_FROM_STREAM)
{
--- 11616,11622 ----
/*
* If the current segment is being streamed from master, calculate how
! * much of the current page we have received already.
*/
if (readSource == XLOG_FROM_STREAM)
{
*************** retry:
*** 11648,11654 ****
}
pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
! if (read(readFile, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
{
char fname[MAXFNAMELEN];
--- 11644,11650 ----
}
pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
! if (read(readFile, readBuf, readLen) != readLen)
{
char fname[MAXFNAMELEN];
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
Antonin Houska <ah@cybertec.at> writes:
> While reading XLogPageRead() I was surprised that readLen variable is set but
> not used in the read() call. Then I realized that it's declared static
> although no other function uses it. Maybe it was used earlier to exit early if
> sufficient amount of data was already read? I think this case is now handled
> by the calling function xlogreader.c:ReadPageInternal().
> I suggest to make the variable local:
Hmm ... I agree that making the variable local is a simple improvement,
but your patch also does this:
> *************** retry:
> *** 11648,11654 ****
> }
>
> pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
> ! if (read(readFile, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
> {
> char fname[MAXFNAMELEN];
>
> --- 11644,11650 ----
> }
>
> pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
> ! if (read(readFile, readBuf, readLen) != readLen)
> {
> char fname[MAXFNAMELEN];
and that I'm less sure is correct.
At one time, I think, readLen told how much data in readBuf was
actually valid. It seems not to be used for that anymore, but
I don't much like the idea that readBuf is only partially filled
but there is *no* persistent state indicating how much is valid.
The existing coding guarantees that the answer is "XLOG_BLCKSZ",
so that's fine, but this change would remove the guarantee.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Antonin Houska <ah@cybertec.at> writes:
> but your patch also does this:
Yes, that should have been a separate diff.
> > *************** retry:
> > *** 11648,11654 ****
> > }
> >
> > pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
> > ! if (read(readFile, readBuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
> > {
> > char fname[MAXFNAMELEN];
> >
> > --- 11644,11650 ----
> > }
> >
> > pgstat_report_wait_start(WAIT_EVENT_WAL_READ);
> > ! if (read(readFile, readBuf, readLen) != readLen)
> > {
> > char fname[MAXFNAMELEN];
>
> and that I'm less sure is correct.
>
> At one time, I think, readLen told how much data in readBuf was
> actually valid. It seems not to be used for that anymore, but
> I don't much like the idea that readBuf is only partially filled
> but there is *no* persistent state indicating how much is valid.
> The existing coding guarantees that the answer is "XLOG_BLCKSZ",
> so that's fine, but this change would remove the guarantee.
XLogPageRead() is a callback of the XLOG reader and that passes reqLen telling
how much data of the page is actually needed in readBuf at the moment. And the
function checks that readLen is high enough:
Assert(reqLen <= readLen);
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
Antonin Houska <ah@cybertec.at> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> At one time, I think, readLen told how much data in readBuf was
>> actually valid. It seems not to be used for that anymore, but
>> I don't much like the idea that readBuf is only partially filled
>> but there is *no* persistent state indicating how much is valid.
>> The existing coding guarantees that the answer is "XLOG_BLCKSZ",
>> so that's fine, but this change would remove the guarantee.
Oh, I withdraw that complaint --- readBuf isn't a persistent data
structure anymore, so there's no need for readLen to be persistent
either.
> XLogPageRead() is a callback of the XLOG reader and that passes reqLen telling
> how much data of the page is actually needed in readBuf at the moment. And the
> function checks that readLen is high enough:
> Assert(reqLen <= readLen);
Hm. Sure would be nice if there were any mention of reqLen in the
function's specification.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote: > Antonin Houska <ah@cybertec.at> writes: > > Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> At one time, I think, readLen told how much data in readBuf was > >> actually valid. It seems not to be used for that anymore, but > >> I don't much like the idea that readBuf is only partially filled > >> but there is *no* persistent state indicating how much is valid. > >> The existing coding guarantees that the answer is "XLOG_BLCKSZ", > >> so that's fine, but this change would remove the guarantee. > > Oh, I withdraw that complaint --- readBuf isn't a persistent data > structure anymore, so there's no need for readLen to be persistent > either. Well, there's yet a problem. Your concern about invalid data in readBuf reminded me of a problem that I reported a few years ago [1]. The problem is hidden as long as all XLOG reader callbacks fetch the whole page. However if readLen is used as an argument of read() in XLogPageRead() *and* if some other conditions that also hide the issue are removed (see [2]), the problem reported in [1] appears to be a live bug. [1] https://www.postgresql.org/message-id/2363.1428573952%40localhost [2] https://www.postgresql.org/message-id/32276.1516290849%40localhost -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com