Re: parallel mode and parallel contexts

On 2015-01-05 11:27:57 -0500, Robert Haas wrote:
> On Sat, Jan 3, 2015 at 7:31 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > * I wonder if parallel contexts shouldn't be tracked via resowners
> 
> That is a good question.  I confess that I'm somewhat fuzzy about
> which things should be tracked via the resowner mechanism vs. which
> things should have their own bespoke bookkeeping.  However, the
> AtEOXact_Parallel() stuff happens well before ResourceOwnerRelease(),
> which makes merging them seem not particularly clean.

I'm not sure either. But I think the current location is wrong anyway -
during AtEOXact_Parallel() before running user defined queries via
AfterTriggerFireDeferred() seems wrong.

> > * combocid.c should error out in parallel mode, as insurance
> 
> Eh, which function?  HeapTupleHeaderGetCmin(),
> HeapTupleHeaderGetCmax(), and AtEOXact_ComboCid() are intended to work
> in parallel mode. HeapTupleHeaderAdjustCmax() could
> Assert(!IsInParallelMode()) but the only calls are in heap_update()
> and heap_delete() which already have error checks, so putting yet
> another elog() there seems like overkill.

To me it seems like a good idea, but whatever.

> > * I doubt it's a good idea to allow heap_insert, heap_inplace_update,
> >   index_insert. I'm not convinced that those will be handled correct and
> >   relaxing restrictions is easier than adding them.
> 
> I'm fine with adding checks to heap_insert() and
> heap_inplace_update().  I'm not sure we really need to add anything to
> index_insert(); how are we going to get there without hitting some
> other prohibited operation first?

I'm not sure. But it's not that hard to imagine that somebody will start
adding codepaths that insert into indexes first. Think upsert.

> > * I'd much rather separate HandleParallelMessageInterrupt() in one
> >   variant that just tells the machinery there's a interrupt (called from
> >   signal handlers) and one that actually handles them. We shouldn't even
> >   consider adding any new code that does allocations, errors and such in
> >   signal handlers. That's just a *bad* idea.
> 
> That's a nice idea, but I didn't invent the way this crap works today.
> ImmediateInterruptOK gets set to true while performing a lock wait,
> and we need to be able to respond to errors while in that state.  I
> think there's got to be a better way to handle that than force every
> asynchronous operation to have to cope with the fact that
> ImmediateInterruptOK may be set or not set, but as of today that's
> what you have to do.

I personally think it's absolutely unacceptable to add any more of
that. That the current mess works is more luck than anything else - and
I'm pretty sure there's several bugs in it. But since I realize I can't
force you to develop a alternative solution, I tried to implement enough
infrastructure to deal with it without too much work.

As far as I can see this could relatively easily be implemented ontop of
the removal of ImmediateInterruptOK in the patch series I posted
yesterday? Afaics you just need to remove most of
+HandleParallelMessageInterrupt() and replace it by a single SetLatch().

> > * I'm not a fan of the shm_toc stuff... Verbose and hard to read. I'd
> >   much rather place a struct there and be careful about not using
> >   pointers. That also would obliviate the need to reserve some ids.
> 
> I don't see how that's going to work with variable-size data
> structures, and a bunch of the things that we need to serialize are
> variable-size.

Meh. Just appending the variable data to the end of the structure and
calculating offsets works just fine.

> Moreover, I'm not really interested in rehashing this
> design again.  I know it's not your favorite; you've said that before.

Well, if I keep having to read code using it, I'll keep being annoyed by
it. I guess we both have to live with that.


Greetings,

Andres Freund