Re: Why are we PageInit'ing buffers in RelationAddExtraBlocks()?

Hi,

On 2019-01-15 17:35:21 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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