On 28/10/2025 10:53, Quan Zongliang wrote:
> On 10/28/25 4:13 PM, Bertrand Drouvot wrote:
>> While working on refactoring some code in [1], one of the changes was:
>>
>> - if (initial_restart_lsn != InvalidXLogRecPtr &&
>> - initial_restart_lsn < oldestLSN)
>> + XLogRecPtr restart_lsn = s->data.restart_lsn;
>> +
>> + if (restart_lsn != InvalidXLogRecPtr &&
>> + restart_lsn < oldestLSN)
>>
>> Sawada-san suggested to use the XLogRecPtrIsInvalid() macro here.
>>
>> But this != InvalidXLogRecPtr check was existing code, so why not
>> consistently use XLogRecPtrIsInvalid() where we check equality
>> against InvalidXLogRecPtr?
>>
>> At the time the current XLogRecPtrIsInvalid() has been introduced
>> (0ab9d1c4b316) all the InvalidXLogRecPtr equality checks were done
>> using XLogRecPtrIsInvalid().
>>
>> But since, it has changed: I looked at it and this is not the case
>> anymore in 20 files.
>>
>> PFA, patches to $SUBJECT. To ease the review, I created one patch
>> per modified file.
>>
>> I suspect the same approach could be applied to some other macros
>> too. Let's start with XLogRecPtrIsInvalid() first.
>
> Agree. This patch has made the code style more consistent. And using
> such macros is also in line with the usual practice. Just like
> OidIsValid and TransactionIdIsValid.
Back in the day, XLogRecPtrIsInvalid() was needed because XLogRecPtr was
a struct. 'x == InvalidXLogRecPtr' simply did not work. I don't see much
need for it nowadays. In fact I wonder if we should go in the other
direction and replace XLogRecPtrIsInvalid(x) calls with 'x ==
InvalidXLogRecPtr'.
It's also a bit cumbersome that we have XLogRecPtrIsInvalid() rather
than XLogRecPtrIsValid(). That's inconsistent with OidIsValid and
TransactionIdInValid, and it leads to an awkward double negative 'if
(!XLogRecPtrIsInvalid(x))' if you want to check that 'x' is valid.
Overall I'm inclined to do nothing. But if anything, perhaps introduce
XLogRecPtrIsValid(x) and switch to that, or replace
XLogRecPtrIsInvalid(x) calls with 'x == InvalidXLogRecPtr'
- Heikki
