Andres Freund <and...@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
