Horton

Thread

Commits

  1. Pass correct count to WALRead().

  2. Add missing check_stack_depth() to some recursive functions

  1. Do away with zero-padding assumption before WALRead()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-13T06:17:06Z

    Hi,

I noticed an assumption [1] at WALRead() call sites expecting the
flushed WAL page to be zero-padded after the flush LSN. I think this
can't always be true as the WAL can get flushed after determining the
flush LSN before reading it from the WAL file using WALRead(). I've
hacked the code up a bit to check if that's true -
https://github.com/BRupireddy2/postgres/tree/ensure_extra_read_WAL_page_is_zero_padded_at_the_end_WIP,
the tests hit the Assert(false); added. Which means, the zero-padding
comment around WALRead() call sites isn't quite right.

I'm wondering why the WALRead() callers are always reading XLOG_BLCKSZ
despite knowing exactly how much to read. Is it to tell the OS to
explicitly fetch the whole page from the disk? If yes, the OS will do
that anyway because the page transfers from disk to OS page cache are
always in terms of disk block sizes, no?

Although, there's no immediate problem with it right now, the
assumption is going to be incorrect when reading WAL from WAL buffers
using WALReadFromBuffers -
https://www.postgresql.org/message-id/CALj2ACV=C1GZT9XQRm4iN1NV1T=hLA_hsGWNx2Y5-G+mSwdhNg@mail.gmail.com.

If we have no reason, can the WALRead() callers just read how much
they want like walsender for physical replication? Attached a patch
for the change.

Thoughts?

[1]
    /*
     * Even though we just determined how much of the page can be validly read
     * as 'count', read the whole page anyway. It's guaranteed to be
     * zero-padded up to the page boundary if it's incomplete.
     */
    if (!WALRead(state, cur_page, targetPagePtr, XLOG_BLCKSZ, tli,
                 &errinfo))

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  2. Re: Do away with zero-padding assumption before WALRead()

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-02-15T10:19:35Z

    Hi,

On Tue, 13 Feb 2024 at 09:17, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> Hi,
>
> I noticed an assumption [1] at WALRead() call sites expecting the
> flushed WAL page to be zero-padded after the flush LSN. I think this
> can't always be true as the WAL can get flushed after determining the
> flush LSN before reading it from the WAL file using WALRead(). I've
> hacked the code up a bit to check if that's true -
> https://github.com/BRupireddy2/postgres/tree/ensure_extra_read_WAL_page_is_zero_padded_at_the_end_WIP,
> the tests hit the Assert(false); added. Which means, the zero-padding
> comment around WALRead() call sites isn't quite right.
>
> I'm wondering why the WALRead() callers are always reading XLOG_BLCKSZ
> despite knowing exactly how much to read. Is it to tell the OS to
> explicitly fetch the whole page from the disk? If yes, the OS will do
> that anyway because the page transfers from disk to OS page cache are
> always in terms of disk block sizes, no?

I am curious about the same. The page size and disk block size could
be different, so the reason could be explicitly fetching the whole
page from the disk as you said. Is this the reason or are there any
other benefits of always reading XLOG_BLCKSZ instead of reading the
sufficient part? I tried to search in older threads and code comments
but I could not find an explanation.

-- 
Regards,
Nazir Bilal Yavuz
Microsoft

  3. Re: Do away with zero-padding assumption before WALRead()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-16T01:40:13Z

    At Tue, 13 Feb 2024 11:47:06 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> Hi,
> 
> I noticed an assumption [1] at WALRead() call sites expecting the
> flushed WAL page to be zero-padded after the flush LSN. I think this
> can't always be true as the WAL can get flushed after determining the
> flush LSN before reading it from the WAL file using WALRead(). I've
> hacked the code up a bit to check if that's true -

Good catch! The comment seems wrong also to me. The subsequent bytes
can be written simultaneously, and it's very normal that there are
unflushed bytes are in OS's page buffer. The objective of the comment
seems to be to declare that there's no need to clear out the remaining
bytes, here. I agree that it's not a problem for now. However, I think
we need two fixes here.

1. It's useless to copy the whole page regardless of the 'count'. It's
  enough to copy only up to the 'count'. The patch looks good in this
  regard.

2. Maybe we need a comment that states the page_read callback
  functions leave garbage bytes beyond the returned count, due to
  partial copying without clearing the unused portion.

> I'm wondering why the WALRead() callers are always reading XLOG_BLCKSZ
> despite knowing exactly how much to read. Is it to tell the OS to
> explicitly fetch the whole page from the disk? If yes, the OS will do
> that anyway because the page transfers from disk to OS page cache are
> always in terms of disk block sizes, no?

If I understand your question correctly, I guess that the whole-page
copy was expected to clear out the remaining bytes, as I mentioned
earlier.

> Although, there's no immediate problem with it right now, the
> assumption is going to be incorrect when reading WAL from WAL buffers
> using WALReadFromBuffers -
> https://www.postgresql.org/message-id/CALj2ACV=C1GZT9XQRm4iN1NV1T=hLA_hsGWNx2Y5-G+mSwdhNg@mail.gmail.com.
>
> If we have no reason, can the WALRead() callers just read how much
> they want like walsender for physical replication? Attached a patch
> for the change.
> 
> Thoughts?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  4. Re: Do away with zero-padding assumption before WALRead()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-16T14:10:00Z

    On Thu, Feb 15, 2024 at 3:49 PM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> > I'm wondering why the WALRead() callers are always reading XLOG_BLCKSZ
> > despite knowing exactly how much to read. Is it to tell the OS to
> > explicitly fetch the whole page from the disk? If yes, the OS will do
> > that anyway because the page transfers from disk to OS page cache are
> > always in terms of disk block sizes, no?
>
> I am curious about the same. The page size and disk block size could
> be different,

Yes, they can be different, but.... (see below)

> so the reason could be explicitly fetching the whole
> page from the disk as you said.

Upon OS page cache miss, the whole page (of disk block size) gets
fetched from disk even if we just read 'count' bytes (< disk block
size), no? This is my understanding about page transfers between disk
and OS page cache.

> Is this the reason or are there any
> other benefits of always reading XLOG_BLCKSZ instead of reading the
> sufficient part? I tried to search in older threads and code comments
> but I could not find an explanation.

FWIW, walsender for physical replication will just read as much as it
wants to read which can range from WAL of size < XLOG_BLCKSZ to
MAX_SEND_SIZE (XLOG_BLCKSZ * 16). I mean, it does not read the whole
page of bytes XLOG_BLCKSZ when it wants to read < XLOG_BLCKSZ.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  5. Re: Do away with zero-padding assumption before WALRead()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-16T14:20:00Z

    On Fri, Feb 16, 2024 at 7:10 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> Good catch! The comment seems wrong also to me. The subsequent bytes
> can be written simultaneously, and it's very normal that there are
> unflushed bytes are in OS's page buffer. The objective of the comment
> seems to be to declare that there's no need to clear out the remaining
> bytes, here. I agree that it's not a problem for now. However, I think
> we need two fixes here.
>
> 1. It's useless to copy the whole page regardless of the 'count'. It's
>   enough to copy only up to the 'count'. The patch looks good in this
>   regard.

Yes, it's not needed to copy the whole page. Per my understanding
about page transfers between disk and OS page cache - upon OS page
cache miss, the whole page (of disk block size) gets fetched from disk
even if we just read 'count' bytes (< disk block size).

> 2. Maybe we need a comment that states the page_read callback
>   functions leave garbage bytes beyond the returned count, due to
>   partial copying without clearing the unused portion.

Isn't the comment around page_read callback at
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/access/xlogreader.h;h=2e9e5f43eb2de1ca9ba81afe76d21357065c61aa;hb=d57b7cc3338e9d9aa1d7c5da1b25a17c5a72dcce#l78
enough?

"The callback shall return the number of bytes read (never more than
XLOG_BLCKSZ), or -1 on failure."

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  6. Re: Do away with zero-padding assumption before WALRead()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-19T02:56:22Z

    At Fri, 16 Feb 2024 19:50:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On Fri, Feb 16, 2024 at 7:10 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > 1. It's useless to copy the whole page regardless of the 'count'. It's
> >   enough to copy only up to the 'count'. The patch looks good in this
> >   regard.
> 
> Yes, it's not needed to copy the whole page. Per my understanding
> about page transfers between disk and OS page cache - upon OS page
> cache miss, the whole page (of disk block size) gets fetched from disk
> even if we just read 'count' bytes (< disk block size).

Right, but with a possibly-different block size. Anyway that behavior
doesn't affect the result of this change. (Could affect performance
hereafter if it were not the case, though..)

> > 2. Maybe we need a comment that states the page_read callback
> >   functions leave garbage bytes beyond the returned count, due to
> >   partial copying without clearing the unused portion.
> 
> Isn't the comment around page_read callback at
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/include/access/xlogreader.h;h=2e9e5f43eb2de1ca9ba81afe76d21357065c61aa;hb=d57b7cc3338e9d9aa1d7c5da1b25a17c5a72dcce#l78
> enough?
> 
> "The callback shall return the number of bytes read (never more than
> XLOG_BLCKSZ), or -1 on failure."

Yeah, perhaps I was overly concerned. The removed comment made me
think that someone could add code relying on the incorrect assumption
that the remaining bytes beyond the returned count are cleared out. On
the flip side, SimpleXLogPageRead always reads a whole page and
returns XLOG_BLCKSZ. However, as you know, the returned buffer doesn't
contain random garbage bytes. Therefore, it's safe as long as the
caller doesn't access beyond the returned count. As a result, the
description you pointed out seems to be enough.

After all, the patch looks good to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  7. Re: Do away with zero-padding assumption before WALRead()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-19T03:02:41Z

    At Mon, 19 Feb 2024 11:56:22 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Yeah, perhaps I was overly concerned. The removed comment made me
> think that someone could add code relying on the incorrect assumption
> that the remaining bytes beyond the returned count are cleared out. On
> the flip side, SimpleXLogPageRead always reads a whole page and
> returns XLOG_BLCKSZ. However, as you know, the returned buffer doesn't
> contain random garbage bytes. Therefore, it's safe as long as the

Forgot to mention that there is a case involving non-initialized
pages, but it doesn't affect the correctness of the description you
pointed out.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  8. Re: Do away with zero-padding assumption before WALRead()

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-19T05:32:39Z

    On Mon, Feb 19, 2024 at 8:26 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> On
> the flip side, SimpleXLogPageRead always reads a whole page and
> returns XLOG_BLCKSZ. However, as you know, the returned buffer doesn't
> contain random garbage bytes.

Is this assumption true when wal_init_zero is off? I think when
wal_init_zero is off, the last few bytes of the last page from the WAL
file may contain garbage bytes i.e. not zero bytes, no?

> Therefore, it's safe as long as the
> caller doesn't access beyond the returned count. As a result, the
> description you pointed out seems to be enough.

Right.

> After all, the patch looks good to me.

Thanks. It was committed -
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=73f0a1326608ac3a7d390706fdeec59fe4dc42c0.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  9. Re: Do away with zero-padding assumption before WALRead()

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2024-02-19T05:47:58Z

    At Mon, 19 Feb 2024 11:02:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> > After all, the patch looks good to me.
> 
> Thanks. It was committed -
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=73f0a1326608ac3a7d390706fdeec59fe4dc42c0.

Yeah. I realied that after I had already sent the mail.. No harm done:p

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center