Subject: Re: [WIP] [B-Tree] Retail IndexTuple deletion
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <425db134-8bba-005c-b59d-56e50de3b41e@postgrespro.ru>
 <CAH2-WzkOmpQ30nRy6=fdQ8Um8MKBRZ9yU6MwEFDQnWcVb3dY8Q@mail.gmail.com>
 <CAH2-Wz=bhuwATGhiQVACbEcVrdGJ-TKyTCg7rZtm1ncRzSuGfw@mail.gmail.com>
 <CAD21AoC-31Fr2JGmCpQPjJNZb-bvH38gURAC7256aJg3jO2C7w@mail.gmail.com>
 <CAH2-Wz=U-m6rDUdj2JcGycX3OT+0588ZCsauCfC7Fdv3_UmKGQ@mail.gmail.com>
 <5affa50e-72a9-aef5-d71f-f5f551e01aaf@postgrespro.ru>
 <CAD21AoAcsz-K-PW0HH+B9awQtyhnqcCDz6yMY8tq9kN6q4dPhg@mail.gmail.com>
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Message-ID: <40c94500-ee09-2d14-169b-c6a7b5d46f9b@postgrespro.ru>
Date: Wed, 27 Jun 2018 09:05:41 +0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: 
 <CAD21AoAcsz-K-PW0HH+B9awQtyhnqcCDz6yMY8tq9kN6q4dPhg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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