22.12.2016 07:19, Amit Kapila:
On Wed, Dec 21, 2016 at 8:46 PM, Anastasia Lubennikova
<lubennikov...@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
