Re: Review: GIN non-intrusive vacuum of posting tree

Hi, Teodor!

2017-03-21 20:32 GMT+05:00 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
> I had a look on patch

That's great, thanks!

>
> /*
> * All subtree is empty - just return TRUE to indicate that parent
> must
> * do a cleanup. Unless we are ROOT an there is way to go upper.
> */
>
> if(isChildHasVoid && !isAnyNonempy && !isRoot)
> return TRUE;
>
> if(isChildHasVoid)
> {
> ...
> ginScanToDelete(gvs, blkno, TRUE, &root,
> InvalidOffsetNumber);
> }
>
> In first 'if' clause I see !isRoot, so second if and corresponding
> ginScanToDelete() could be called only for root in posting tree.

No, second conditional code will be called for any subtree, which
contains totally empty subtree. That check !isRoot covers case when
the entire posting tree should be erased: we cannot just quit out of
recursive cleanup, we have to make a scan here, starting from root.

Probably, variable isChildHasVoid has a bit confusing name. This flag
indicates that some subtree:
1. Had empty pages
2. Did not bother deleting them, because there is a chance that it is
a part of a bigger empty subtree.
May be it'd be better to call the variable "someChildIsVoidSubtree".

>just remove lock for cleanup over ginVacuumPostingTreeLeaves() and if it returns that tree contains empty page then lock the whole posting tree to do ginScanToDelete() work.

It is, indeed, viable approach. But currently proposed patch is
capable of dealing with small page deletes without much of locking
fuss, even in 4-5 level trees.

How do you think, which way should we take?

Best regards, Andrey Borodin.