Thread: Lazy allocation of pages required for verifying FPI consistency

Lazy allocation of pages required for verifying FPI consistency

From
Bharath Rupireddy
Date:
Hi,

Postgres verifies consistency of FPI from WAL record with the replayed
page during recovery in verifyBackupPageConsistency() when either
wal_consistency_checking for the resource manager is enabled or a WAL
record with XLR_CHECK_CONSISTENCY flag is inserted. While doing so, it
uses two intermediate pages primary_image_masked (FPI from WAL record)
and replay_image_masked (replayed page) which are dynamically
allocated (palloc'd) before the recovery starts, however, they're not
used unless verifyBackupPageConsistency() is called. And these
variables are palloc'd here for getting MAXALIGNed memory as opposed
to static char arrays. Since verifyBackupPageConsistency() gets called
conditionally only when the WAL record has the XLR_CHECK_CONSISTENCY
flag set, it's a waste of memory for these two page variables.

I propose to statically allocate these two pages using PGAlignedBlock
structure lazily in verifyBackupPageConsistency() to not waste dynamic
memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.

Thoughts?

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

Attachment

Re: Lazy allocation of pages required for verifying FPI consistency

From
Kyotaro Horiguchi
Date:
At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> I propose to statically allocate these two pages using PGAlignedBlock
> structure lazily in verifyBackupPageConsistency() to not waste dynamic
> memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.
> 
> Thoughts?

IMHO, it's a bit scaring to me to push down the execution stack by
that large size. I tend to choose the (current) possible memory
wasting only on startup process than digging stack deeply.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Lazy allocation of pages required for verifying FPI consistency

From
Julien Rouhaud
Date:
On Thu, Jan 12, 2023 at 4:29 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> > I propose to statically allocate these two pages using PGAlignedBlock
> > structure lazily in verifyBackupPageConsistency() to not waste dynamic
> > memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.
> >
> > Thoughts?
>
> IMHO, it's a bit scaring to me to push down the execution stack by
> that large size. I tend to choose the (current) possible memory
> wasting only on startup process than digging stack deeply.

+1



Re: Lazy allocation of pages required for verifying FPI consistency

From
Bharath Rupireddy
Date:
On Thu, Jan 12, 2023 at 1:59 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Mon, 9 Jan 2023 20:00:00 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> > I propose to statically allocate these two pages using PGAlignedBlock
> > structure lazily in verifyBackupPageConsistency() to not waste dynamic
> > memory worth 2*BLCKSZ bytes. I'm attaching a small patch herewith.
> >
> > Thoughts?
>
> IMHO, it's a bit scaring to me to push down the execution stack by
> that large size. I tend to choose the (current) possible memory
> wasting only on startup process than digging stack deeply.

On the contrary, PGAlignedBlock is being used elsewhere in the code;
some of them are hot paths. verifyBackupPageConsistency() is not
something that gets called always i.e. WAL consistency checks are done
conditionally - when either one enables wal_consistency_checking for
the rmgr or the WAL record is flagged with
XLR_CHECK_CONSISTENCY (core doesn't do, it's an external module, if
any, do that).

I really don't see much of a problem in allocating them statically and
pushing closer to where they're being used. If this really concerns,
at the least, the dynamic allocation needs to be pushed to
verifyBackupPageConsistency() IMO with if (first_time) { allocate two
blocks with palloc} and use them. This at least saves some memory on
the heap for most of the servers out there.

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



Re: Lazy allocation of pages required for verifying FPI consistency

From
Michael Paquier
Date:
On Thu, Jan 12, 2023 at 04:37:38PM +0800, Julien Rouhaud wrote:
> On Thu, Jan 12, 2023 at 4:29 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
>> IMHO, it's a bit scaring to me to push down the execution stack by
>> that large size. I tend to choose the (current) possible memory
>> wasting only on startup process than digging stack deeply.
>
> +1

Indeed.  I agree to leave that be.
--
Michael

Attachment

Re: Lazy allocation of pages required for verifying FPI consistency

From
Kyotaro Horiguchi
Date:
At Thu, 12 Jan 2023 15:02:25 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> On the contrary, PGAlignedBlock is being used elsewhere in the code;

I noticed it and had the same feeling, and thought that they don't
justify to do the same at other places.

> some of them are hot paths. verifyBackupPageConsistency() is not
> something that gets called always i.e. WAL consistency checks are done
> conditionally - when either one enables wal_consistency_checking for
> the rmgr or the WAL record is flagged with
> XLR_CHECK_CONSISTENCY (core doesn't do, it's an external module, if
> any, do that).

Right. So we could allocate them at the first use as below, but...

> I really don't see much of a problem in allocating them statically and
> pushing closer to where they're being used. If this really concerns,
> at the least, the dynamic allocation needs to be pushed to
> verifyBackupPageConsistency() IMO with if (first_time) { allocate two
> blocks with palloc} and use them. This at least saves some memory on
> the heap for most of the servers out there.

Yeah, we could do that. But as I mentioned before, that happens only
on startup thus it can be said that that's not worth bothering.  On
the other hand I don't think it's great to waste 16kB * max_backends
memory especially when it is clearly recognized and easily avoidable.

I guess the reason for the code is more or less that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: Lazy allocation of pages required for verifying FPI consistency

From
Michael Paquier
Date:
On Mon, Jan 16, 2023 at 10:52:43AM +0900, Kyotaro Horiguchi wrote:
> Yeah, we could do that. But as I mentioned before, that happens only
> on startup thus it can be said that that's not worth bothering.  On
> the other hand I don't think it's great to waste 16kB * max_backends
> memory especially when it is clearly recognized and easily avoidable.

Memory's cheap, but basically nobody would use these except
developers..

> I guess the reason for the code is more or less that.

The original discussion spreads across these threads:
https://www.postgresql.org/message-id/CAB7nPqR0jzhF%3DU4AXLm%2BcmaE4J-HkUzbaRXtg%2B6ieERTqr%3Dpcg%40mail.gmail.com
https://www.postgresql.org/message-id/CAGz5QC%2B_CNcDJkkmDyPg2zJ_R8AtEg1KyYADbU6B673RaTySAg%40mail.gmail.com

There was a specific point about using static buffers from me, though
these would not have been aligned as of the lack of PGAlignedBlock
back in 2017 which is why palloc() was used.  That should be around
here:
https://www.postgresql.org/message-id/CAB7nPqR=OcojLCP=1Ho6Zo312CKzUZU8d4aJO+VvpUYV-waU_Q@mail.gmail.com
--
Michael

Attachment