Re: pg_logical_slot_get_changes waits continously for a partial WAL record spanning across 2 pages

On Fri, 27 Jun 2025 at 07:05, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jun 26, 2025 at 05:25:42PM +0530, vignesh C wrote:
> > On Thu, 26 Jun 2025 at 06:22, Michael Paquier <michael@paquier.xyz> wrote:
> >> So you are suggesting the addition of an extra ReadPageInternal() that
> >> forces a read of only the read, perform the checks on the header, then
> >> read the rest.  After reading SizeOfXLogShortPHD worth of data,
> >> shouldn't the checks on xlp_rem_len be done a bit earlier than what
> >> you are proposing in this patch?
> >
> > Modified
>
> It seems to me that this assert can be moved after the second page
> read:
> Assert(SizeOfXLogShortPHD <= readOff);
>
> Coming back to the point of Kuroda-san about performance, did you do
> some checks related to that and did you measure any difference?  I
> suspect none of that because in most cases we are just going to fetch
> the next page and we would trigger the fast-exit path of
> ReadPageInternal() on the second call when fetching the rest.  I still
> need to get an idea of all that by myself, probably with various
> lengths of logical message records.
>
> Perhaps this code could be improved in the future with less page
> reads.  Anyway, what you are doing here is simple enough that it is a
> no-brainer for the back-branches because we are just forcing our way
> through with a new short header validation, so logically that's sound
> as far as I can see.

I was unable to reproduce the same test failure on the PG17 branch,
even after running the test around 500 times. However, on the master
branch, the failure consistently reproduces approximately once in
every 50 runs. I also noticed that while the buildfarm has reported
multiple failures for this test for the master branch, none of them
appear to be on the PG17 branch. I'm not yet sure why this discrepancy
exists.

Regards,
Vignesh