Thread: Lazy allocation of pages required for verifying FPI consistency
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
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
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
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
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
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
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