Re: parallel mode and parallel contexts

On 2015-03-18 12:02:07 -0400, Robert Haas wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index cb6f8a3..173f0ba 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -2234,6 +2234,17 @@ static HeapTuple
>  heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
>                      CommandId cid, int options)
>  {
> +    /*
> +     * For now, parallel operations are required to be strictly read-only.
> +     * Unlike heap_update() and heap_delete(), an insert should never create
> +     * a combo CID, so it might be possible to relax this restrction, but
> +     * not without more thought and testing.
> +     */
> +    if (IsInParallelMode())
> +        ereport(ERROR,
> +                (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
> +                 errmsg("cannot insert tuples during a parallel operation")));
> +

Minor nitpick: Should we perhaps move the ereport to a separate
function? Akin to PreventTransactionChain()? Seems a bit nicer to not
have the same message everywhere.

> +void
> +DestroyParallelContext(ParallelContext *pcxt)
> +{
> +    int        i;
> +
> +    /*
> +     * Be careful about order of operations here!  We remove the parallel
> +     * context from the list before we do anything else; otherwise, if an
> +     * error occurs during a subsequent step, we might try to nuke it again
> +     * from AtEOXact_Parallel or AtEOSubXact_Parallel.
> +     */
> +    dlist_delete(&pcxt->node);

Hm. I'm wondering about this. What if we actually fail below? Like in
dsm_detach() or it's callbacks? Then we'll enter abort again at some
point (during proc_exit at the latest) but will not wait for the child
workers. Right?
> +/*
> + * End-of-subtransaction cleanup for parallel contexts.
> + *
> + * Currently, it's forbidden to enter or leave a subtransaction while
> + * parallel mode is in effect, so we could just blow away everything.  But
> + * we may want to relax that restriction in the future, so this code
> + * contemplates that there may be multiple subtransaction IDs in pcxt_list.
> + */
> +void
> +AtEOSubXact_Parallel(bool isCommit, SubTransactionId mySubId)
> +{
> +    while (!dlist_is_empty(&pcxt_list))
> +    {
> +        ParallelContext *pcxt;
> +
> +        pcxt = dlist_head_element(ParallelContext, node, &pcxt_list);
> +        if (pcxt->subid != mySubId)
> +            break;
> +        if (isCommit)
> +            elog(WARNING, "leaked parallel context");
> +        DestroyParallelContext(pcxt);
> +    }
> +}

> +/*
> + * End-of-transaction cleanup for parallel contexts.
> + */
> +void
> +AtEOXact_Parallel(bool isCommit)
> +{
> +    while (!dlist_is_empty(&pcxt_list))
> +    {
> +        ParallelContext *pcxt;
> +
> +        pcxt = dlist_head_element(ParallelContext, node, &pcxt_list);
> +        if (isCommit)
> +            elog(WARNING, "leaked parallel context");
> +        DestroyParallelContext(pcxt);
> +    }
> +}

Is there any reason to treat the isCommit case as a warning? To me that
sounds like a pretty much guaranteed programming error. If your're going
to argue that a couple resource leakage warnings do similarly: I don't
think that counts for that much. For one e.g. a leaked tupdesc has much
less consequences, for another IIRC those warnings were introduced
after the fact.

> + * When running as a parallel worker, we place only a single
> + * TransactionStateData is placed on the parallel worker's
> + * state stack,

'we place .. is placed'

>  /*
> + *    EnterParallelMode
> + */
> +void
> +EnterParallelMode(void)
> +{
> +    TransactionState s = CurrentTransactionState;
> +
> +    Assert(s->parallelModeLevel >= 0);
> +
> +    ++s->parallelModeLevel;
> +}
> +
> +/*
> + *    ExitParallelMode
> + */
> +void
> +ExitParallelMode(void)
> +{
> +    TransactionState s = CurrentTransactionState;
> +
> +    Assert(s->parallelModeLevel > 0);
> +    Assert(s->parallelModeLevel > 1 || !ParallelContextActive());
> +
> +    --s->parallelModeLevel;
> +
> +    if (s->parallelModeLevel == 0)
> +        CheckForRetainedParallelLocks();
> +}

Hm. Is it actually ok for nested parallel mode to retain locks on their
own? Won't that pose a problem? Or did you do it that way just because
we don't have more state? If so that deserves a comment explaining that
htat's the case and why that's acceptable.
> @@ -1769,6 +1904,9 @@ CommitTransaction(void)
>  {
>      TransactionState s = CurrentTransactionState;
>      TransactionId latestXid;
> +    bool        parallel;
> +
> +    parallel = (s->blockState == TBLOCK_PARALLEL_INPROGRESS);

> +    /* If we might have parallel workers, clean them up now. */
> +    if (IsInParallelMode())
> +    {
> +        CheckForRetainedParallelLocks();
> +        AtEOXact_Parallel(true);
> +        s->parallelModeLevel = 0;
> +    }

'parallel' looks strange when we're also, rightly so, do stuff like
checking IsInParallelMode(). How about naming it is_parallel_worker or
something?


Sorry, ran out of concentration here. It's been a long day.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services