From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
Date: Tue, 15 Jan 2019 17:35:21 -0500
Message-ID: <11587.1547591721@sss.pgh.pa.us> (raw)
In-Reply-To: <20190115204428.kpqmk3ix6cenr7gi@alap3.anarazel.de>
References: <20181219083945.6khtgm36mivonhva@alap3.anarazel.de>
<CA+TgmoZRwKbf6q7TXEpwMX_vdjfeGEKudR8mofnmTwZg6O9=kg@mail.gmail.com>
<20181219223736.4k2lhqoj34aaxj3m@alap3.anarazel.de>
<CA+TgmoaJ67VCefuhkd0fdJiWb64bqBtkbZazbVm6a0wimY0fRQ@mail.gmail.com>
<13071.1545266373@sss.pgh.pa.us>
<20181220005636.cowkudhiu666mhdi@alap3.anarazel.de>
<20181220230411.zzwioucrg7d3ld2m@alap3.anarazel.de>
<20190115204428.kpqmk3ix6cenr7gi@alap3.anarazel.de>
Andres Freund <andres@anarazel.de> writes:
>> Looking at the surrounding code made me wonder about the wisdom of
>> entering empty pages as all-visible and all-frozen into the VM. That'll
>> mean we'll never re-discover them on a primary, after promotion. There's
>> no mechanism to add such pages to the FSM on a standby (in contrast to
>> e.g. pages where tuples are modified), as vacuum will never visit that
>> page again. Now obviously it has the advantage of avoiding
>> re-processing the page in the next vacuum, but is that really an
>> important goal? If there's frequent vacuums, there got to be a fair
>> amount of modifications, which ought to lead to re-use of such pages at
>> a not too far away point?
> Any comments on the approach in this patch?
I agree with the concept of postponing page init till we're actually
going to do something with the page. However, the patch seems to need
some polish:
* There's a comment in RelationAddExtraBlocks, just above where you
changed, that
* Extend by one page. This should generally match the main-line
* extension code in RelationGetBufferForTuple, except that we hold
* the relation extension lock throughout.
This seems to be falsified by this patch, in that one of the two paths
does PageInit and the other doesn't.
* s/unitialized pages/uninitialized pages/
* This bit in vacuumlazy seems unnecessarily confusing:
+ Size freespace = 0;
...
+ if (GetRecordedFreeSpace(onerel, blkno) == 0)
+ freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
+
+ if (freespace > 0)
+ {
+ RecordPageWithFreeSpace(onerel, blkno, freespace);
I'd write that as just
+ if (GetRecordedFreeSpace(onerel, blkno) == 0)
+ {
+ Size freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
+ RecordPageWithFreeSpace(onerel, blkno, freespace);
I tend to agree that the DEBUG message isn't very necessary, or at least
could be lower than DEBUG1.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, robertmhaas@gmail.com, amit.kapila16@gmail.com
Subject: Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?
In-Reply-To: <11587.1547591721@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox