Re: Minmax indexes - Mailing list pgsql-hackers

On 07/10/2014 12:41 AM, Alvaro Herrera wrote:
> Heikki Linnakangas wrote:
>> On 06/23/2014 08:07 PM, Alvaro Herrera wrote:
>
>> I feel that the below would nevertheless be simpler:
>>
>>>> I wonder if it would be simpler to just always store the revmap
>>>> pages in the beginning of the index, before any other pages. Finding
>>>> the revmap page would then be just as easy as with a separate fork.
>>>> When the table/index is extended so that a new revmap page is
>>>> needed, move the existing page at that block out of the way. Locking
>>>> needs some consideration, but I think it would be feasible and
>>>> simpler than you have now.
>>>
>>> Moving index items around is not easy, because you'd have to adjust the
>>> revmap to rewrite the item pointers.
>>
>> Hmm. Two alternative schemes come to mind:
>>
>> 1. Move each index tuple off the page individually, updating the
>> revmap while you do it, until the page is empty. Updating the revmap
>> for a single index tuple isn't difficult; you have to do it anyway
>> when an index tuple is replaced. (MMTuples don't contain a heap
>> block number ATM, but IMHO they should, see below)
>>
>> 2. Store the new block number of the page that you moved out of the
>> way in the revmap page, and leave the revmap pointers unchanged. The
>> revmap pointers can be updated later, lazily.
>>
>> Both of those seem pretty straightforward.
>
> The trouble I have with moving blocks around to make space, is that it
> would cause the index to have periodic hiccups to make room for the new
> revmap pages.  One nice property that these indexes are supposed to have
> is that the effect into insertion times should be pretty minimal.  That
> would cease to be the case if we have to do your proposed block moves.

Approach 2 above is fairly quick, quick enough that no-one would notice 
the "hiccup". Moving the tuples individually (approach 1) would be slower.

>>>> ISTM that when the old tuple cannot be updated in-place, the new
>>>> index tuple is inserted with mm_doinsert(), but the old tuple is
>>>> never deleted.
>>>
>>> It's deleted by the next vacuum.
>>
>> Ah I see. Vacuum reads the whole index, and builds an in-memory hash
>> table that contains an ItemPointerData for every tuple in the index.
>> Doesn't that require a lot of memory, for a large index? That might
>> be acceptable - you ought to have plenty of RAM if you're pushing
>> around multi-terabyte tables - but it would nevertheless be nice to
>> not have a hard requirement for something as essential as vacuum.
>
> I guess if you're expecting that pages_per_range=1 is a common case,
> yeah it might become an issue eventually.

Not sure, but I find it easier to think of the patch that way. In any 
case, it would be nice to avoid the problem, even if it's not common.

> One idea I just had is to
> have a bit for each index tuple, which is set whenever the revmap no
> longer points to it.  That way, vacuuming is much easier: just scan the
> index and delete all tuples having that bit set.

The bit needs to be set atomically with the insertion of the new tuple, 
so why not just remove the old tuple right away?

>> Wouldn't it be simpler to remove the old tuple atomically with
>> inserting the new tuple and updating the revmap? Or at least mark
>> the old tuple as deletable, so that vacuum can just delete it,
>> without building the large hash table to determine that it's
>> deletable.
>
> Yes, it might be simpler, but it'd require dirtying more pages on
> insertions (and holding more page-level locks, for longer.  Not good for
> concurrent access).

I wouldn't worry much about the performance and concurrency of this 
operation. Remember that the majority of updates are expected to not 
have to update the index, otherwise the minmax index will degenerate 
quickly and performance will suck anyway. And even when updating the 
index is needed, in most cases the new tuple fits on the same page, 
after removing the old one. So the case where you have to insert a new 
index tuple, remove old one (or mark it dead), and update the revmap to 
point to the new tuple, is rare.

>> I'm quite surprised by the use of LockTuple on the index tuples. I
>> think the main reason for needing that is the fact that MMTuple
>> doesn't store the heap (range) block number that the tuple points
>> to: LockTuple is required to ensure that the tuple doesn't go away
>> while a scan is following a pointer from the revmap to it. If the
>> MMTuple contained the BlockNumber, a scan could check that and go
>> back to the revmap if it doesn't match. Alternatively, you could
>> keep the revmap page locked when you follow a pointer to the regular
>> index page.
>
> There's the intention that these accesses be kept as concurrent as
> possible; this is why we don't want to block the whole page.  Locking
> individual TIDs is fine in this case (which is not in SELECT FOR UPDATE)
> because we can only lock a single tuple in any one index scan, so
> there's no unbounded growth of the lock table.
>
> I prefer not to have BlockNumbers in index tuples, because that would
> make them larger for not much gain.  That data would mostly be
> redundant, and would be necessary only for vacuuming.

Don't underestimate the value of easier debugging. I wouldn't worry much 
about shaving four bytes from the tuple, these indexes are tiny in any 
case. Keep it simple at first, and optimize later if necessary.

In fact, I'd suggest just using normal IndexTuple instead of the custom 
MMTuple struct, store the block number in t_tid and leave offset number 
field of that unused. That wastes 2 more bytes per tuple, but that's 
insignificant too. I feel that it probably would be worth it just to 
keep thing simple, and you'd e.g. be able to use index_deform_tuple() as is.

- Heikki