Re: Size of Path nodes

On Sat, Dec 5, 2015 at 2:30 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Fri, Dec 4, 2015 at 12:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > So over in my private branch where I've been fooling around with upper
> > planner pathification, I've been sweating bullets to avoid enlarging the
> > size of struct Path, because I was aware that common path types like
> > IndexPath and AppendPath were already a power-of-2 size (at least on
> > 64-bit machines), meaning that palloc'ing them would cost twice as much
> > space if I added any fields.
> >
> > When I got around to merging up to HEAD, I found this in commit f0661c4e8:
> >
>
> > While I'm bitching about this: a field added to a struct as fundamental
> > as Path ought to have a pretty well-defined meaning, and this does not
> > as far as I can tell.  There was certainly no documentation worthy of
> > the name added by the above commit.  What is the semantic difference
> > between a Path with this flag set and the identical Path without?
> > Likewise for struct Plan?
>
> /me crawls into a hole.
>
> Well, funny story about that.  The parallel sequential scan code
> forgot to make that flag actually do what it was budgeted to do, but
> it turns out not to matter as long as the only thing that can be done
> in parallel is a sequential scan.  So there's no live bug right now,
> and the parallel join patch adds the missing code to make that flag
> actually meaningful.  So I'm not very surprised that you found the
> current state of things stupid. The reason it's needed is because the
> parallel join patch generates plans like this:
>
> Gather
> -> Hash Join
>   -> Parallel Seq Scan
>   -> Hash
>     -> Seq Scan
>
> The idea of this plan is that each worker builds a hash table on the
> inner rel, and then reads a subset of the outer rel and performs the
> join for the subset of rows which it reads.  Then the Gather brings
> all the worker results together in the leader.  Well, when I initially
> coded this, it wasn't working properly, and I couldn't for the life of
> me figure out why.
>
> Eventually, after hours of debugging, I realized that while I'd added
> this parallel_aware flag with the idea of distinguishing between the
> outer seq scan (which needs to be split between the workers) from the
> inner seq scan (which needs to be executed to completion by every
> worker) there was nothing in the code which would actually cause it to
> work like that.  The inner seq scan was behaving as if it were
> parallel - that is, it returned only a subset of the rows in each
> worker - even though the parallel_aware flag was not set.  I had
> thought that there was some logic in execParallel.c or, uh, somewhere,
> which would give that flag meaning but it turns out that, in master,
> there isn't.  So the parallel join patch, which is otherwise just an
> optimizer patch, also adds the missing bits to execParallel.c.  Maybe
> I should have committed that separately as a bug fix.
>
> To try to clarify a little further, execParallel.c contains three
> functions that make passes over the PlanState tree.  The first one is
> made before creating the parallel DSM, and its purpose is to estimate
> the amount of space that each plan node will require in the DSM (it
> can overestimate, but it can't underestimate).  The second one is made
> after creating the parallel DSM, and gives plan nodes a chance to
> initialize the space they estimated they would use (or some lesser
> amount).  The third one is made in each worker, and gives plan nodes a
> chance to fish out a pointer to the space previously initialized in
> the master.  All of this processing is of course skipped for plan
> nodes that have no parallel smarts.  But the parallel-aware flag is
> supposed to allow these steps to be skipped even for a node that does
> have parallel smarts, so that those smarts can in effect be shut off
> when they're not desired.
>  
>
> Of course, it would be possible to rejigger things to avoid having
> this flag in every path.  Currently, SeqScans are the only plan type
> that is ever parallel-aware, so we could put the plan in only that
> path type.  However, I expect to want to expand the set of
> parallel-aware paths pretty soon.  In particular, I want to add at
> least AppendPath and IndexPath to the list.
>

To add to whatever has been said above, intention of adding that flag

was also to avoid adding new nodes for parallelism.  Basically modify

the existing nodes (like SeqScan) to take care of whatever is needed

for parallel execution.  Now fortunately, not much is needed for Seq Scan

(except for explain part and distinguishing backward scan), but I think

that will not be the case going forward when we need to make other nodes

like Append parallel aware.  The need and idea for this flag has been

proposed and discussed on parallel seq scan thread [1].