Re: GiST VACUUM - Mailing list pgsql-hackers

Hi, Heikki!

Thanks for looking into the patch!

> 11 июля 2018 г., в 0:07, Heikki Linnakangas <hlinnaka@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
pagesas deleted, only leaf pages. However, we should have something like this in the leaf-page branch, to deal with the
casethat 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
pageit encounters. In the second phase, it scans every leaf page of that parent, not only those leaves that were seen
asempty. 
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
inB-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
whenI understood it.... 

>
> If you crash in the middle of gistbulkdelete(), after it has removed the downlink from the parent, but before it has
markedthe 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
complexfor little purpose. Will add comments on possible orphan pages. 

Many thanks! I hope to post updated patch series this week.


Best regards, Andrey Borodin.