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

On Wed, Dec 19, 2018 at 2:09 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> The zheap patchset, even after being based on pluggable storage,
> currently has the following condition in RelationAddExtraBlocks():
>                 if (RelationStorageIsZHeap(relation))
>                 {
>                         Assert(BufferGetBlockNumber(buffer) != ZHEAP_METAPAGE);
>                         ZheapInitPage(page, BufferGetPageSize(buffer));
>                         freespace = PageGetZHeapFreeSpace(page);
>                 }
>                 else
>                 {
>                         PageInit(page, BufferGetPageSize(buffer), 0);
>                         freespace = PageGetHeapFreeSpace(page);
>                 }
>
> I.e. it initializes the page differently when zheap is used versus
> heap.
>
> Thinking about whether it's worth to allow to extend that function in an
> extensible manner made me wonder:  Is it actually a good idea to
> initialize the page at that point, including marking it dirty?
>
> As far as I can tell that that has several downsides:
> - Dirtying the buffer for initialization will cause potentially
>   superfluous IO, with no interesting data in the write except for a
>   newly initialized page.
> - As there's no sort of interlock, it's entirely possible that, after a
>   crash, the blocks will come up empty, but with the FSM returning it as
>   as empty, so that path would be good to support anyway.
> - It adds heap specific code to a routine that otherwise could be
>   generic for different table access methods
>

IIUC, your proposal is to remove page initialization and
MarkBufferDirty from RelationAddExtraBlocks(), but record them in FSM.
Is my understanding correct, if so, I don't see any problem with that
and as you have mentioned, it will be generally advantageous as well?

> It seems to me, this could be optimized by *not* initializing the page,
> and having a PageIsNew(), check at the places that check whether the
> page is new, and initialize it in that case.
>

makes sense to me.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com