Thread: pgsql: xlog.c: Remove global variables ReadRecPtr and EndRecPtr.
xlog.c: Remove global variables ReadRecPtr and EndRecPtr. In most places, the variables necessarily store the same value as the eponymous members of the XLogReaderState that we use during WAL replay, because ReadRecord() assigns the values from the structure members to the global variables just after XLogReadRecord() returns. However, XLogBeginRead() adjusts the structure members but not the global variables, so after XLogBeginRead() and before the completion of XLogReadRecord() the values can differ. Otherwise, they must be identical. According to my analysis, the only place where either variable is referenced at a point where it might not have the same value as the structure member is the refrence to EndRecPtr within XLogPageRead. Therefore, at every other place where we are using the global variable, we can just switch to using the structure member instead, and remove the global variable. However, we can, and in fact should, do this in XLogPageRead() as well, because at that point in the code, the global variable will actually store the start of the record we want to read - either because it's where the last WAL record ended, or because the read position has been changed using XLogBeginRead since the last record was read. The structure member, on the other hand, will already have been updated to point to the end of the record we just read. Elsewhere, the latter is what we use as an argument to emode_for_corrupt_record(), so we should do the same here. This part of the patch is perhaps a bug fix, but I don't think it has any important consequences, so no back-patch. The point here is just to continue to whittle down the entirely excessive use of global variables in xlog.c. Discussion: http://postgr.es/m/CA+Tgmoao96EuNeSPd+hspRKcsCddu=b1h-QNRuKfY8VmfNQdfg@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/d2ddfa681db27a138acb63c8defa8cc6fa588922 Modified Files -------------- src/backend/access/transam/xlog.c | 53 ++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 29 deletions(-)
Hi, On 2021-11-24 16:27:58 +0000, Robert Haas wrote: > xlog.c: Remove global variables ReadRecPtr and EndRecPtr. This fails when building with WAL_DEBUG. There's remaining Read/EndRecPtr references in #ifdef WAL_DEBUG sections... Greetings, Andres Freund
Hi, On 2021-11-24 15:12:06 -0800, Andres Freund wrote: > This fails when building with WAL_DEBUG. There's remaining Read/EndRecPtr > references in #ifdef WAL_DEBUG sections... Pushed the obvious fix for that. Somehow thought I'd seen more compile failure than the one WAL_DEBUG...
On Wed, Nov 24, 2021 at 8:01 PM Andres Freund <andres@anarazel.de> wrote: > Pushed the obvious fix for that. Somehow thought I'd seen more compile failure > than the one WAL_DEBUG... Hmm, thanks. I guess i put too much trust in the compiler. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Nov 24, 2021 at 8:01 PM Andres Freund <andres@anarazel.de> wrote:
>> Pushed the obvious fix for that. Somehow thought I'd seen more compile failure
>> than the one WAL_DEBUG...
> Hmm, thanks. I guess i put too much trust in the compiler.
My approach to such patches is always "in grep we trust, all others
pay cash". Even without #ifdef issues, you are highly likely to
miss comments that need to be updated.
regards, tom lane
On Thu, Nov 25, 2021 at 11:02 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > On Wed, Nov 24, 2021 at 8:01 PM Andres Freund <andres@anarazel.de> wrote: > >> Pushed the obvious fix for that. Somehow thought I'd seen more compile failure > >> than the one WAL_DEBUG... > > > Hmm, thanks. I guess i put too much trust in the compiler. > > My approach to such patches is always "in grep we trust, all others > pay cash". Even without #ifdef issues, you are highly likely to > miss comments that need to be updated. Right. I didn't rely completely on grep and did spend a lot of time looking through the code. But sometimes my eyes glaze over and I get sloppy after too much time working on one thing, and this seems to have been one of those times. Fortunately, it seems to have been only a minor oversight. -- Robert Haas EDB: http://www.enterprisedb.com
Hi,
On November 26, 2021 5:53:31 PM PST, Andres Freund <andres@anarazel.de> wrote:
>Hi,
>
>On November 26, 2021 7:24:15 AM PST, Robert Haas <robertmhaas@gmail.com> wrote:
>>Fortunately, it seems to have been only a minor oversight.
>
Agreed.
I wonder if we should turn some of these ifdefs into something boiling down to if (0) { ifdef body}... No need to make
itimpossible for the compiler to help us in most of these cases...
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.