Re: GSoC 2017: weekly progress reports (week 6)

<https://mailtrack.io/> Sent with Mailtrack
<https://mailtrack.io/install?source=signature&lang=en&referral=shubhambaraiss(at)gmail(dot)com&idSignature=22>
<#>

On 28 September 2017 at 15:49, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru
> wrote:

> On Thu, Sep 28, 2017 at 12:45 AM, Alexander Korotkov <
> a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
>> On Wed, Aug 9, 2017 at 6:30 PM, Shubham Barai <shubhambaraiss(at)gmail(dot)com>
>> wrote:
>>
>>> Please find the updated patch for predicate locking in gin index here.
>>>
>>> There was a small issue in the previous patch. I didn't consider the case
>>> where only root page exists in the tree, and there is a predicate lock
>>> on it,
>>> and it gets split.
>>>
>>> If we treat the original page as a left page and create a new root and
>>> right
>>> page, then we just need to copy a predicate lock from the left to the
>>> right
>>> page (this is the case in B-tree).
>>>
>>> But if we treat the original page as a root and create a new left and
>>> right
>>> page, then we need to copy a predicate lock on both new pages (in the
>>> case of rum and gin).
>>>
>>> link to updated code and tests: https://github.com/shub
>>> hambaraiss/postgres/commit/6172639a104785f051cb4aa0d511c58f2bae65a6
>>>
>>
>> I've assigned to review this patch. First of all I'd like to understand
>> general idea of this patch.
>>
>> As I get, you're placing predicate locks to both entry tree leaf pages
>> and posting tree leaf pages. But, GIN implements so called "fast scan"
>> technique which allows it to skip some leaf pages of posting tree when
>> these pages are guaranteed to not contain matching item pointers. Wherein
>> the particular order of posting list scan and skip depends of their
>> estimated size (so it's a kind of coincidence).
>>
>> But thinking about this more generally, I found that proposed locking
>> scheme is redundant. Currently when entry has posting tree, you're locking
>> both:
>> 1) entry tree leaf page containing pointer to posting tree,
>> 2) leaf pages of corresponding posting tree.
>> Therefore conflicting transactions accessing same entry would anyway
>> conflict while accessing the same entry tree leaf page. So, there is no
>> necessity to lock posting tree leaf pages at all. Alternatively, if entry
>> has posting tree, you can skip locking entry tree leaf page and lock
>> posting tree leaf pages instead.
>>
>
> I'd like to note that I had following warnings during compilation using
> clang.
>
> gininsert.c:519:47: warning: incompatible pointer to integer conversion
>> passing 'void *' to parameter of type 'Buffer' (aka 'int')
>> [-Wint-conversion]
>> CheckForSerializableConflictIn(index, NULL, NULL);
>> ^~~~
>> /Applications/Xcode.app/Contents/Developer/Toolchains/
>> XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.0.0/include/stddef.h:105:16:
>> note: expanded from macro 'NULL'
>> # define NULL ((void*)0)
>> ^~~~~~~~~~
>> ../../../../src/include/storage/predicate.h:64:87: note: passing
>> argument to parameter 'buffer' here
>> extern void CheckForSerializableConflictIn(Relation relation, HeapTuple
>> tuple, Buffer buffer);
>>
>> ^
>> 1 warning generated.
>> ginvacuum.c:163:2: warning: implicit declaration of function
>> 'PredicateLockPageCombine' is invalid in C99 [-Wimplicit-function-
>> declaration]
>> PredicateLockPageCombine(gvs->index, deleteBlkno, rightlink);
>> ^
>> 1 warning generated.
>
>
> Also, I tried to remove predicate locks from posting tree leafs. At least
> isolation tests passed correctly after this change.
>
> However, after telegram discussion with Andrew Borodin, we decided that it
> would be better to do predicate locking and conflict checking for posting
> tree leafs, but skip that for entry tree leafs (in the case when entry has
> posting tree). That would give us more granular locking and less false
> positives.
>
>
>
>
Hi Alexander,

I have made changes according to your suggestions. Please have a look at
the updated patch.
I am also considering your suggestions for my other patches also. But, I
will need some time to
make changes as I am currently busy doing my master's.

Kind Regards,
Shubham