Re: [WIP] [B-Tree] Retail IndexTuple deletion

On 26.06.2018 15:31, Masahiko Sawada wrote:
> On Fri, Jun 22, 2018 at 8:24 PM, Andrey V. Lepikhov
> <a.lepikhov@postgrespro.ru> wrote:
>> Hi,
>> According to your feedback, i develop second version of the patch.
>> In this version:
>> 1. High-level functions index_beginscan(), index_rescan() not used. Tree
>> descent made by _bt_search(). _bt_binsrch() used for positioning on the
>> page.
>> 2. TID list introduced in amtargetdelete() interface. Now only one tree
>> descent needed for deletion all tid's from the list with equal scan key
>> value - logical duplicates deletion problem.
>> 3. Only one WAL record for index tuple deletion per leaf page per
>> amtargetdelete() call.
>> 4. VACUUM can sort TID list preliminary for more quick search of duplicates.
>>
>> Background worker will come later.
>>
>>
> 
> Thank you for updating patches! Here is some comments for the latest patch.
> 
> +static void
> +quick_vacuum_index(Relation irel, Relation hrel,
> +                                  IndexBulkDeleteResult **overall_stats,
> +                                  LVRelStats *vacrelstats)
> +{
> (snip)
> +       /*
> +        * Collect statistical info
> +        */
> +       lazy_cleanup_index(irel, *overall_stats, vacrelstats);
> +}
> 
> I think that we should not call lazy_cleanup_index at the end of
> quick_vacuum_index because we call it multiple times during a lazy
> vacuum and index statistics can be changed during vacuum. We already
> call lazy_cleanup_index at the end of lazy_scan_heap.
> 
Ok

> bttargetdelete doesn't delete btree pages even if pages become empty.
> I think we should do that. Otherwise empty page never be recycled. But
> please note that if we delete btree pages during bttargetdelete,
> recyclable pages might not be recycled. That is, if we choose the
> target deletion method every time then the deleted-but-not-recycled
> pages could never be touched, unless reaching
> vacuum_cleanup_index_scale_factor. So I think we need to either run
> bulk-deletion method or do cleanup index before btpo.xact wraparound.
> 
> +               ivinfo.indexRelation = irel;
> +               ivinfo.heapRelation = hrel;
> +               qsort((void *)vacrelstats->dead_tuples,
> vacrelstats->num_dead_tuples, sizeof(ItemPointerData),
> tid_comparator);
> 
Ok. I think caller of bttargetdelete() must decide when to make index 
cleanup.

> I think the sorting vacrelstats->dead_tuples is not necessary because
> garbage TIDs  are stored in a sorted order.
> 
Sorting was introduced because I keep in mind background worker and more 
flexible cleaning strategies, not only full tuple-by-tuple relation and 
block scan.
Caller of bttargetdelete() can set info->isSorted to prevent sorting 
operation.

> Regards,
> 
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center
> 

-- 
Andrey Lepikhov
Postgres Professional:
https://postgrespro.com
The Russian Postgres Company