Re: Unnecessary static variable?

Antonin Houska <ah(at)cybertec(dot)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