Обсуждение: Use XLogRecPtrIsValid() instead of negated XLogRecPtrIsInvalid

Hi,

This small cleanup patch updates src/bin/pg_waldump/archive_waldump.c
to use the recently introduced XLogRecPtrIsValid() helper instead of
negating XLogRecPtrIsInvalid(). The current code uses double-negative
checks such as:
Assert(!XLogRecPtrIsInvalid(privateInfo->startptr));
if (!XLogRecPtrIsInvalid(privateInfo->endptr))

This patch changes them to:
Assert(XLogRecPtrIsValid(privateInfo->startptr));
if (XLogRecPtrIsValid(privateInfo->endptr))

This improves readability without changing behavior. The attached
patch has the changes for the same.

Regards,
Vignesh

On Fri, Apr 10, 2026 at 3:11 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Hi,
>
> This small cleanup patch updates src/bin/pg_waldump/archive_waldump.c
> to use the recently introduced XLogRecPtrIsValid() helper instead of
> negating XLogRecPtrIsInvalid(). The current code uses double-negative
> checks such as:
> Assert(!XLogRecPtrIsInvalid(privateInfo->startptr));
> if (!XLogRecPtrIsInvalid(privateInfo->endptr))
>
> This patch changes them to:
> Assert(XLogRecPtrIsValid(privateInfo->startptr));
> if (XLogRecPtrIsValid(privateInfo->endptr))
>
> This improves readability without changing behavior. The attached
> patch has the changes for the same.

Commit a2b02293bc6 switched various places to use XLogRecPtrIsValid(),
but it looks like later commits accidentally introduced uses of
XLogRecPtrIsInvalid() again. So +1 for this change.

Also, that commit replaced direct comparisons with InvalidXLogRecPtr with
XLogRecPtrIsValid(). I noticed two such comparisons [1]. Should these be
updated as well?

Regards,

[1]
$ git grep -E "[=\!]= InvalidXLogRecPtr"
src/backend/commands/repack_worker.c:   Assert(ctx->reader->EndRecPtr
!= InvalidXLogRecPtr);
src/backend/replication/walreceiver.c:  applyPtr = (latestApplyPtr ==
InvalidXLogRecPtr) ?

--
Fujii Masao

On Fri, 10 Apr 2026 at 20:16, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, Apr 10, 2026 at 3:11 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > Hi,
> >
> > This small cleanup patch updates src/bin/pg_waldump/archive_waldump.c
> > to use the recently introduced XLogRecPtrIsValid() helper instead of
> > negating XLogRecPtrIsInvalid(). The current code uses double-negative
> > checks such as:
> > Assert(!XLogRecPtrIsInvalid(privateInfo->startptr));
> > if (!XLogRecPtrIsInvalid(privateInfo->endptr))
> >
> > This patch changes them to:
> > Assert(XLogRecPtrIsValid(privateInfo->startptr));
> > if (XLogRecPtrIsValid(privateInfo->endptr))
> >
> > This improves readability without changing behavior. The attached
> > patch has the changes for the same.
>
> Commit a2b02293bc6 switched various places to use XLogRecPtrIsValid(),
> but it looks like later commits accidentally introduced uses of
> XLogRecPtrIsInvalid() again. So +1 for this change.
>
> Also, that commit replaced direct comparisons with InvalidXLogRecPtr with
> XLogRecPtrIsValid(). I noticed two such comparisons [1]. Should these be
> updated as well?

I felt these also should be updated, the attached v2 version patch
includes the changes for the same.

Regards,
Vignesh

On Mon, Apr 13, 2026 at 4:10 PM vignesh C <vignesh21@gmail.com> wrote:
> I felt these also should be updated, the attached v2 version patch
> includes the changes for the same.

Thanks for updating the patch!

-       applyPtr = (latestApplyPtr == InvalidXLogRecPtr) ?
+       applyPtr = (XLogRecPtrIsInvalid(latestApplyPtr)) ?

XLogRecPtrIsValid() should be used here, instead?

Regards,

--
Fujii Masao

在 2026/4/16 12:11, Fujii Masao 写道:
> On Mon, Apr 13, 2026 at 4:10 PM vignesh C <vignesh21@gmail.com> wrote:
>> I felt these also should be updated, the attached v2 version patch
>> includes the changes for the same.
> Thanks for updating the patch!
>
> -       applyPtr = (latestApplyPtr == InvalidXLogRecPtr) ?
> +       applyPtr = (XLogRecPtrIsInvalid(latestApplyPtr)) ?
>
> XLogRecPtrIsValid() should be used here, instead?
>
> Regards,
>

Yeah, I was about to raise the same comment, and Fujii-san beat me. I believe it should be XLogRecPtrIsValid().

Otherwise, the patch looks good to me.

Regards,
Xiaopeng Wang

On Thu, 16 Apr 2026 at 09:42, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Mon, Apr 13, 2026 at 4:10 PM vignesh C <vignesh21@gmail.com> wrote:
> > I felt these also should be updated, the attached v2 version patch
> > includes the changes for the same.
>
> Thanks for updating the patch!
>
> -       applyPtr = (latestApplyPtr == InvalidXLogRecPtr) ?
> +       applyPtr = (XLogRecPtrIsInvalid(latestApplyPtr)) ?
>
> XLogRecPtrIsValid() should be used here, instead?

Yes, that seems better, the attached v3 version patch has the changes
for the same.

Regards,
Vignesh

On Thu, Apr 16, 2026 at 4:25 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 16 Apr 2026 at 09:42, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Mon, Apr 13, 2026 at 4:10 PM vignesh C <vignesh21@gmail.com> wrote:
> > > I felt these also should be updated, the attached v2 version patch
> > > includes the changes for the same.
> >
> > Thanks for updating the patch!
> >
> > -       applyPtr = (latestApplyPtr == InvalidXLogRecPtr) ?
> > +       applyPtr = (XLogRecPtrIsInvalid(latestApplyPtr)) ?
> >
> > XLogRecPtrIsValid() should be used here, instead?

The outer parentheses do not seem to be needed, as
XLogRecPtrIsInvalid() already includes them.

Other than that, the v3 patch looks good to me.

Regards,
Amul

On Thu, Apr 16, 2026 at 9:31 PM Amul Sul <sulamul@gmail.com> wrote:
> The outer parentheses do not seem to be needed, as
> XLogRecPtrIsInvalid() already includes them.
>
> Other than that, the v3 patch looks good to me.

I've updated the patch based on Amul's suggestion and pushed it. Thanks!

Regards,

--
Fujii Masao

On Thu, 16 Apr 2026 at 19:36, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Apr 16, 2026 at 9:31 PM Amul Sul <sulamul@gmail.com> wrote:
> > The outer parentheses do not seem to be needed, as
> > XLogRecPtrIsInvalid() already includes them.
> >
> > Other than that, the v3 patch looks good to me.
>
> I've updated the patch based on Amul's suggestion and pushed it. Thanks!

Thanks for pushing the patch.

Regards,
Vignesh