On Wed, Dec 19, 2018 at 2:09 PM Andres Freund <and...@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