Re: [HACKERS] Parallel Index Scans

On Sat, Jan 21, 2017 at 1:15 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 9:29 AM, Amit Kapila <amit.kapila16@gmail.com> 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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers