Subject: Re: [WIP] [B-Tree] Retail IndexTuple deletion
To: Peter Geoghegan <pg@bowt.ie>
References: <425db134-8bba-005c-b59d-56e50de3b41e@postgrespro.ru>
 <CAH2-WzkOmpQ30nRy6=fdQ8Um8MKBRZ9yU6MwEFDQnWcVb3dY8Q@mail.gmail.com>
 <CAH2-Wz=bhuwATGhiQVACbEcVrdGJ-TKyTCg7rZtm1ncRzSuGfw@mail.gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Message-ID: <0b74aa3e-3d76-397b-d14d-a4a24c3dfb1d@postgrespro.ru>
Date: Wed, 20 Jun 2018 09:04:02 +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: 
 <CAH2-Wz=bhuwATGhiQVACbEcVrdGJ-TKyTCg7rZtm1ncRzSuGfw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 19.06.2018 04:05, Peter Geoghegan wrote:
> On Mon, Jun 18, 2018 at 2:54 PM, Peter Geoghegan <pg@bowt.ie> wrote:
>> On Sun, Jun 17, 2018 at 9:39 PM, Andrey V. Lepikhov
>> <a.lepikhov@postgrespro.ru> wrote:
>>> Patch '0001-retail-indextuple-deletion' introduce new function
>>> amtargetdelete() in access method interface. Patch
>>> '0002-quick-vacuum-strategy' implements this function for an alternative
>>> strategy of lazy index vacuum, called 'Quick Vacuum'.
>>
>> My compiler shows the following warnings:
> 
> Some real feedback:
> 
> What we probably want to end up with here is new lazyvacuum.c code
> that does processing for one heap page (and associated indexes) that
> is really just a "partial" lazy vacuum. Though it won't do things like
> advance relfrozenxid, it will do pruning for the heap page, index
> tuple killing, and finally heap tuple killing. It will do all of these
> things reliably, just like traditional lazy vacuum. This will be what
> your background worker eventually uses.
> 
It is final goal of the patch.

> I doubt that you can use routines like index_beginscan() within
> bttargetdelete() at all. I think that you need something closer to
> _bt_doinsert() or _bt_pagedel(), that manages its own scan (your code
> should probably live in nbtpage.c). It does not make sense to teach
> external, generic routines like index_beginscan() about heap TID being
> an implicit final index attribute, which is one reason for this (I'm
> assuming that this patch relies on my own patch).  Another reason is
> that you need to hold an exclusive buffer lock at the point that you
> identify the tuple to be killed, until the point that you actually
> kill it. You don't do that now.
> 
> IOW, the approach you've taken in bttargetdelete() isn't quite correct
> because you imagine that it's okay to occasionally "lose" the index
> tuple that you originally found, and just move on. That needs to be
> 100% reliable, or else we'll end up with index tuples that point to
> the wrong heap tuples in rare cases with concurrent insertions. As I
> said, we want a "partial" lazy vacuum here, which must mean that it's
> reliable. Note that _bt_pagedel() actually calls _bt_search() when it
> deletes a page. Your patch will not be the first patch that makes
> nbtree vacuuming do an index scan. You should be managing your own
> insertion scan key, much like _bt_pagedel() does. If you use my patch,
> _bt_search() can be taught to look for a specific heap TID.
> 
Agree with this notes. Corrections will made in the next version of the 
patch.

> Finally, doing things this way would let you delete multiple
> duplicates in one shot, as I described in an earlier e-mail. Only a
> single descent of the tree is needed to delete quite a few index
> tuples, provided that they all happen to be logical duplicates. Again,
> your background worker will take advantage of this.
> 
It is very interesting idea. According to this, I plan to change 
bttargetdelete() interface as follows:
IndexTargetDeleteStats*
amtargetdelete(IndexTargetDeleteInfo *info,
			   IndexTargetDeleteStats *stats,
			   Datum *values, bool *isnull);
where structure IndexTargetDeleteInfo contains a TID list of dead heap 
tuples. All index entries, corresponding to this list, may be deleted 
(or only some of it) by one call of amtargetdelete() function with 
single descent of the tree.

> This code does not follow the Postgres style:
> 
>> -       else
>> +       }
>> +       else {
>> +           if (rootoffnum != latestdead)
>> +               heap_prune_record_unused(prstate, latestdead);
>>              heap_prune_record_redirect(prstate, rootoffnum, chainitems[i]);
>> +       }
>>      }
> 
> Please be more careful about that. I find it very distracting.
> 
Done

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