Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()? - Mailing list pgsql-hackers

Hi,

On 2019-01-15 17:35:21 -0500, Tom Lane wrote:
> 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:

Yea, as I'd written in an earlier message, it was really meant as a
prototype to see whether there's buyin to the change.


> * 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);

Fixed, thanks for looking!


> I tend to agree that the DEBUG message isn't very necessary, or at least
> could be lower than DEBUG1.

After a bit of back and forth waffling, I've removed it now.

Besides a fair bit of comment changes the latest version has just one
functional change: lazy_check_needs_freeze() doesn't indicate requiring
freezing for new pages anymore, if we can't get a cleanup lock on those,
it's about to be written to, and we'd not do more than enter it into the
FSM.

Greetings,

Andres Freund