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