Hi, Heikki!
Thanks for looking into the patch!
> 11 июля 2018 г., в 0:07, Heikki Linnakangas <hlinn...@iki.fi> написал(а):
>
> I'm now looking at the first patch in this series, to allow completely empty
> GiST pages to be recycled. I've got some questions:
>
>> --- a/src/backend/access/gist/gist.c
>> +++ b/src/backend/access/gist/gist.c
>> @@ -700,6 +700,13 @@ gistdoinsert(Relation r, IndexTuple itup, Size
>> freespace, GISTSTATE *giststate)
>> GISTInsertStack *item;
>> OffsetNumber downlinkoffnum;
>> + if(GistPageIsDeleted(stack->page))
>> + {
>> + UnlockReleaseBuffer(stack->buffer);
>> + xlocked = false;
>> + state.stack = stack = stack->parent;
>> + continue;
>> + }
>> downlinkoffnum = gistchoose(state.r, stack->page, itup,
>> giststate);
>> iid = PageGetItemId(stack->page, downlinkoffnum);
>> idxtuple = (IndexTuple) PageGetItem(stack->page, iid);
>
> This seems misplaced. This code deals with internal pages, and as far as I
> can see, this patch never marks internal pages as deleted, only leaf pages.
> However, we should have something like this in the leaf-page branch, to deal
> with the case that an insertion lands on a page that was concurrently deleted.
That's a bug. Will fix this.
> Did you have any tests, where an insertion runs concurrently with vacuum,
> that would exercise this?
Yes, I've tried to test this, but, obviously, not enough. I'll think more about
how to deal with it.
>
> The code in gistbulkdelete() seems pretty expensive. In the first phase, it
> records the parent of every empty leaf page it encounters. In the second
> phase, it scans every leaf page of that parent, not only those leaves that
> were seen as empty.
Yes, in first patch there is simplest gistbulkdelete(), second patch will
remember line pointers of downlinks to empty leafs.
>
> I'm a bit wary of using pd_prune_xid for the checks to determine if a deleted
> page can be recycled yet. In heap pages, pd_prune_xid is just a hint, but
> here it's used for a critical check. This seems to be the same mechanism we
> use in B-trees, but in B-trees, we store the XID in BTPageOpaqueData.xact,
> not pd_prune_xid. Also, in B-trees, we use ReadNewTransactionId() to set it,
> not GetCurrentTransactionId(). See comments in _bt_unlink_halfdead_page() for
> explanation. This patch is missing any comments to explain how this works in
> GiST.
Will look into this. I remember it was OK half a year ago, but now it is clear
to me that I had to document that part when I understood it....
>
> If you crash in the middle of gistbulkdelete(), after it has removed the
> downlink from the parent, but before it has marked the leaf page as deleted,
> the leaf page is "leaked". I think that's acceptable, but a comment at least
> would be good.
I was considering doing reverse-split (page merge) concurrency like in Lanin
and Shasha's paper, but it is just too complex for little purpose. Will add
comments on possible orphan pages.
Many thanks! I hope to post updated patch series this week.
Best regards, Andrey Borodin.
