On Sat, Jan 21, 2017 at 1:15 AM, Robert Haas <[email protected]> wrote: > On Fri, Jan 20, 2017 at 9:29 AM, Amit Kapila <[email protected]> wrote: >> Sure, if scan keys have changed then we can't continue, but this is >> the case where runtime keys are first time initialized. >> >> if (node->iss_NumRuntimeKeys != 0 && !node->iss_RuntimeKeysReady) >> >> In the above check, the second part of the check >> (!node->iss_RuntimeKeysReady) ensures that it is for the first time. >> Now, let me give you an example to explain what bad can happen if we >> allow resetting parallel scan in this case. Consider a query like >> select * from t1 where c1 < parallel_index(10);, in this if we allow >> resetting parallel scan descriptor during first time initialization of >> runtime keys, it can easily corrupt the parallel scan state. Suppose >> leader has taken the lead and is scanning some page and worker reaches >> to initialize its keys in ExecReScanIndexScan(), if worker resets the >> parallel scan, then it will corrupt the state of the parallel scan >> state. > > Hmm, I see. So the problem if I understand it correctly is that every > participating process needs to update the backend-private state for > the runtime keys and only one of those processes can update the shared > state. But in the case of a "real" rescan, even the shared state > needs to be reset. OK, makes sense. >
Exactly.
> Why does btparallelrescan cater to the case where scan->parallel_scan
> == NULL? I would assume it should never get called in that case.
>
Okay, will modify the patch accordingly.
> Also, I think ExecReScanIndexScan needs significantly better comments.
> After some thought I see what's it's doing - mostly anyway - but I was
> quite confused at first. I still don't completely understand why it
> needs this if-test:
>
> + /* reset (parallel) index scan */
> + if (node->iss_ScanDesc)
> + {
>
I have mentioned the reason towards the end of the e-mail [1] (Refer
line, This is required because ..). Basically, this is required to
make plans like below work sanely.
Nested Loop
-> Seq Scan on a
-> Gather
-> Parallel Index Scan on b
Index Cond: b.x = 15
I understand that such plans don't make much sense, but we do support
them and I have seen somewhat similar plan getting select in TPC-H
benchmark Let me know if this needs more explanation.
>
> I think it's a good idea to put all three of those functions together
> in the listing, similar to what we did in
> 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06 for FDWs. After all they are
> closely related in purpose, and it may be easiest to understand if
> they are next to each other in the listing. I suggest that we move
> them to the end in IndexAmRoutine similar to the way FdwRoutine was
> done; in other words, my idea is to make the structure consistent with
> the way that I revised the documentation instead of making the
> documentation consistent with the order you picked for the structure
> members. What I like about that is that it gives a good opportunity
> to include some general remarks on parallel index scans in a central
> place, as I did in that patch. Also, it makes it easier for people
> who care about parallel index scans to find all of the related things
> (since they are together) and for people who don't care about them to
> ignore it all (for the same reason). What do you think about that
> approach?
>
Sounds sensible. Updated patch based on that approach is attached. I
will rebase the remaining work based on this patch and send them
separately.
[1] -
https://www.postgresql.org/message-id/CAA4eK1%2BnBiCxtxcNuzpaiN%2BnrRrRB5YDgoaqb3hyn%3DYUxL-%2BOw%40mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
parallel-generic-index-scan.1.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
