Re: Parallel Index Scans

22.12.2016 07:19, Amit Kapila:
> On Wed, Dec 21, 2016 at 8:46 PM, Anastasia Lubennikova
> <lubennikovaav(at)gmail(dot)com> wrote:
>> The following review has been posted through the commitfest application:
>> make installcheck-world: tested, passed
>> Implements feature: tested, passed
>> Spec compliant: tested, passed
>> Documentation: tested, passed
>>
>> Hi, thank you for the patch.
>> Results are very promising. Do you see any drawbacks of this feature or something that requires more testing?
>>
> I think you can focus on the handling of array scan keys for testing.
> In general, one of my colleagues has shown interest in testing this
> patch and I think he has tested as well but never posted his findings.
> I will request him to share his findings and what kind of tests he has
> done, if any.

Check please code related to buffer locking and pinning once again.
I got the warning. Here are the steps to reproduce it:
Except "autovacuum = off" config is default.

pgbench -i -s 100 test
pgbench -c 10 -T 120 test

SELECT count(aid) FROM pgbench_accounts
WHERE aid > 1000 AND aid < 900000 AND bid > 800 AND bid < 900;
WARNING: buffer refcount leak: [8297] (rel=base/12289/16459,
blockNum=2469, flags=0x93800000, refcount=1 1)
count
-------
0
(1 row)

postgres=# select 16459::regclass;
regclass
-----------------------
pgbench_accounts_pkey

>> 2. Don't you mind to rename 'amestimateparallelscan' to let's say 'amparallelscan_spacerequired'
>> or something like this?
> Sure, I am open to other names, but IMHO, lets keep "estimate" in the
> name to keep it consistent with other parallel stuff. Refer
> execParallel.c to see how widely this word is used.
>
>> As far as I understand there is nothing to estimate, we know this size
>> for sure. I guess that you've chosen this name because of 'heap_parallelscan_estimate'.
>> But now it looks similar to 'amestimate' which refers to indexscan cost for optimizer.
>> That leads to the next question.
>>
> Do you mean 'amcostestimate'? If you want we can rename it
> amparallelscanestimate to be consistent with amcostestimate.

I think that 'amparallelscanestimate' seems less ambiguous than
amestimateparallelscan.
But it's up to you. There are enough comments to understand the purpose
of this field.
>
>> And why do you call it pageStatus? What does it have to do with page?
>>
> During scan this tells us whether next page is available for scan.
> Another option could be to name it as scanStatus, but not sure if that
> is better. Do you think if we add a comment like "indicates whether
> next page is available for scan" for this variable then it will be
> clear?

Yes, I think it describes the flag better.
>> 5. Comment for _bt_parallel_seize() says:
>> "False indicates that we have reached the end of scan for
>> current scankeys and for that we return block number as P_NONE."
>>
>> What is the reason to check (blkno == P_NONE) after checking (status == false)
>> in _bt_first() (see code below)? If comment is correct
>> we'll never reach _bt_parallel_done()
>>
>> + blkno = _bt_parallel_seize(scan, &status);
>> + if (status == false)
>> + {
>> + BTScanPosInvalidate(so->currPos);
>> + return false;
>> + }
>> + else if (blkno == P_NONE)
>> + {
>> + _bt_parallel_done(scan);
>> + BTScanPosInvalidate(so->currPos);
>> + return false;
>> + }
>>
> The first time master backend or worker hits last page (calls this
> API), it will return P_NONE and after that any worker tries to fetch
> next page, it will return status as false. I think we can expand a
> comment to explain it clearly. Let me know, if you need more
> clarification, I can explain it in detail.
>
Got it,
I think you can add this explanation to the comment for
_bt_parallel_seize().

>> 6. To avoid code duplication, I would wrap this into the function
>>
>> + /* initialize moreLeft/moreRight appropriately for scan direction */
>> + if (ScanDirectionIsForward(dir))
>> + {
>> + so->currPos.moreLeft = false;
>> + so->currPos.moreRight = true;
>> + }
>> + else
>> + {
>> + so->currPos.moreLeft = true;
>> + so->currPos.moreRight = false;
>> + }
>> + so->numKilled = 0; /* just paranoia */
>> + so->markItemIndex = -1; /* ditto */
>>
> Okay, I think we can write a separate function (probably inline
> function) for above.
>
>> And after that we can also get rid of _bt_parallel_readpage() which only
>> bring another level of indirection to the code.
>>
> See, this function is responsible for multiple actions like
> initializing moreLeft/moreRight positions, reading next page, dropping
> the lock/pin. So replicating all these actions in the caller will
> make the code in caller less readable as compared to now. Consider
> this point and let me know your view on same.

Thank you for clarification, now I agree with your implementation.
I've just missed that we also handle lock in this function.

Performance results with 2 parallel workers are about 1.5-3 times
better, just like in your tests.
So, no doubt, this feature will be useful.
But I'm trying to find the worst cases for this feature. And I suppose
we should test parallel index scans with
concurrent insertions. More parallel readers we have, higher the
concurrency.
I doubt that it can significantly decrease performance, because number
of parallel readers is not that big,
but it is worth testing.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company