Hi!
PFA v5 of the patch series.
> 11 июля 2018 г., в 0:07, Heikki Linnakangas <hlinnaka@iki.fi> написал(а):
>
> 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. Did you have any tests, where an insertion runs
concurrentlywith vacuum, that would exercise this?
That bug could manifest only in case of crash between removing downlinks and marking pages deleted. I do not know how
totest this reliably.
Internal pages are locked before leafs and locks are coupled. No cuncurrent backend can see downlinks to pages being
deleted,unless crash happens.
I've replaced code covering this situation into leaf code path and added comment.
>
> 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.
It is fixed in second patch of the series.
>
> 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.
I've replaced usage of GetCurrentTransactionId() with ReadNewTransactionId() and added explanation of what is going on.
Also,I've added comments about that pd_prune_xid usage. There is no other use in GiST for this field. And there is no
otherroom to place this xid on a page without pg_upgrade.
>
> 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.
Added explanatory comment between WAL-logging downlink removal and marking pages deleted.
Thank you for reviewing the patch!
Best regards, Andrey Borodin.