Re: [HACKERS] GSoC 2017: weekly progress reports (week 6)

On 28/03/18 19:53, Teodor Sigaev wrote:
> Hi!
>
> I slightly modified test for clean demonstration of difference between
> fastupdate on and off. Also I added CheckForSerializableConflictIn() to
> fastupdate off codepath, but only in case of non-empty pending list.
>
> The next question what I see: why do not we lock entry leaf pages in some cases?

Why should we?

> As I understand, scan should lock any visited page, but now it's true only for
> posting tree. Seems, it also should lock pages in entry tree because concurrent
> procesess could add new entries which could be matched by partial search, for
> example. BTW, partial search (see collectMatchBitmap()) locks correctly entry
> tree, but regular startScanEntry() doesn't lock entry page in case of posting
> tree, only in case of posting list.
I think this needs some high-level comments or README to explain how the
locking works. It seems pretty ad hoc at the moment. And incorrect.

1. Why do we lock all posting tree pages, even though they all represent
the same value? Isn't it enough to lock the root of the posting tree?

2. Why do we lock any posting tree pages at all, if we lock the entry
tree page anyway? Isn't the lock on the entry tree page sufficient to
cover the key value?

3. Why do we *not* lock the entry leaf page, if there is no match? We
still need a lock to remember that we probed for that value and there
was no match, so that we conflict with a tuple that might be inserted later.

At least #3 is a bug. The attached patch adds an isolation test that
demonstrates it. #1 and #2 are weird, and cause unnecessary locking, so
I think we should fix those too, even if they don't lead to incorrect
results.

Remember, the purpose of predicate locks is to lock key ranges, not
physical pages or tuples in the index. We use leaf pages as handy
shortcut for "any key value that would belong on this page", but it is
just an implementation detail.

I took a stab at fixing those issues, as well as the bug when fastupdate
is turned on concurrently. Does the attached patch look sane to you?

- Heikki