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

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:

* 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