Re: Parallel Index Scans

On Wed, Jan 18, 2017 at 8:03 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Why do we need separate AM support for index_parallelrescan()? Why
>> can't index_rescan() cover that case?
>
> The reason is that sometime index_rescan is called when we have to
> just update runtime scankeys in index and we don't want to reset
> parallel scan for that.

Why not? I see the code, but the comments don't seem to offer any
justification for that. And it seems wrong to me. If the scan keys
change, surely that only makes sense if we restart the scan. You
can't just blindly continue the same scan if the keys have changed,
can you?

I think the reason that this isn't breaking for you is that it's
difficult or impossible to get a parallel index scan someplace where
the keys would change at runtime. Normally, the parallel scan is on
the driving relation, and so there are no runtime keys. We currently
have no way for a parallel scan to occur on the inner side of a nested
loop unless there's an intervening Gather node - and in that case the
keys can't change without relaunching the workers. It's hard to see
how it could work otherwise. For example, suppose you had something
like this:

Gather
-> Nested Loop
-> Parallel Seq Scan on a
-> Hash Join
-> Seq Scan on b
-> Parallel Shared Hash
-> Parallel Index Scan on c
Index Cond: c.x = a.x

Well, the problem here is that there's nothing to ensure that various
workers who are cooperating to build the hash table all have the same
value for a.x, nor is there any thing to ensure that they'll all get
done with the shared hash table at the same time. So this is just
chaos. I think we have to disallow this kind of plan as nonsensical.
Given that, I'm not sure a reset of the runtime keys can ever really
happen. Have you investigated this?

I extracted the generic portions of this infrastructure (i.e. not the
btree-specific stuff) and spent some time working on it today. The
big thing I fixed was the documentation, which you added in a fairly
illogical part of the file. You had all of the new functions for
supporting parallel scans in a section that explicitly says it's for
mandatory functions, and which precedes the section on regular
non-parallel scans. I moved the documentation for all three new
methods to the same place, added some explanation of parallel scans in
general, and rewrote the descriptions for the individual functions to
be more clear. Also, in indexam.c, I adjusted things to use
RELATION_CHECKS in a couple of places, did some work on comments and
coding style, and fixed a place that should have used the new
OffsetToPointer macro but instead hand-rolled the thing with the casts
backwards. Adding an integer to a value of type "void *" does not
work on all compilers. The patch I ended up with is attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company