a raft of parallelism-related bug fixes - Mailing list pgsql-hackers

My recent commit of the Gather executor node has made it relatively
simple to write code that does an end-to-end test of all of the
parallelism-relate commits which have thus far gone into the tree.
Specifically, what I've done is hacked the planner to push a
single-copy Gather node on top of every plan that is thought to be
parallel-safe, and then run 'make check'.  This turned up bugs in
nearly every parallelism-related commit that has thus far gone into
the tree, which is a little depressing, especially because some of
them are what we've taken to calling brown paper bag bugs.  The good
news is that, with one or two exceptions, these are pretty much just
trivial oversights which are simple to fix, rather than any sort of
deeper design issue.  Attached are 14 patches.  Patches #1-#4 are
essential for testing purposes but are not proposed for commit,
although some of the code they contain may eventually become part of
other patches which are proposed for commit.  Patches #5-#12 are
largely boring patches fixing fairly uninteresting mistakes; I propose
to commit these on an expedited basis.  Patches #13-14 are also
proposed for commit but seem to me to be more in need of review.  With
all of these patches, I can now get a clean 'make check' run, although
I think there are a few bugs remaining to be fixed because some of my
colleagues still experience misbehavior even with all of these patches
applied.  The patch stack is also posted here; the branch is subject
to rebasing:

http://git.postgresql.org/gitweb/?p=users/rhaas/postgres.git;a=shortlog;h=refs/heads/gathertest

Here follows an overview of each individual patch (see also commit
messages within).

== For Testing Only ==

0001-Test-code.patch is the basic test code.  In addition to pushing a
Gather node on top of apparently-safe parallel plans, it also ignores
that Gather node when generating EXPLAIN output and suppressing
parallel context in error messages, changes which are essential to
getting the regression tests to pass.  I'm wondering if the parallel
context error ought to be GUC-controlled, defaulting to on but capable
of being enabled on request.

0002-contain_parallel_unsafe-check_parallel_safety.patch and
0003-Temporary-hack-to-reduce-testing-failures.patch arrange NOT to
put Gather nodes on top of plans that contain parallel-restricted
operations or refer to temporary tables.  Although such things can
exist in a parallel plan, they must be above every Gather node, not
beneath it.  Here, the Gather node is being placed (strictly for
testing purposes) at the very top, so we must not insert it at all if
these things are present.

0004-Partial-group-locking-implementation.patch is a partial
implementation of group locking.  I found that without this, the
regression tests hang frequently, and a clean run is impossible. This
patch doesn't modify the deadlock detector, and it doesn't take any
account of locks that should be mutually exclusive even as between
members of a parallel group, but it's enough for a clean regression
test run.  We will need a full solution to this problem soon enough,
but right now I am only using this to find such unrelated bugs as we
may have.

== Proposed For Commit ==

0005-Don-t-send-protocol-messages-to-a-shm_mq-that-no-lon.patch fixes
a problem in the parallel worker shutdown sequence: a background
worker can choose to redirect messages that would normally be sent to
a client to a shm_mq, and parallel workers always do this.  But if the
worker generates a message after the DSM has been detached, it causes
a server crash.

0006-Transfer-current-command-counter-ID-to-parallel-work.patch fixes
a problem in the code used to set up a parallel worker's transaction
state.  The command counter is presently not copied to the worker.
This is awfully embarrassing and should have been caught in the
testing of the parallel mode/contexts patch, but I got overly focused
on the stuff stored inside TransactionStateData.  Don't shoot.

0007-Tighten-up-application-of-parallel-mode-checks.patch fixes
another problem with the parallel mode checks, which are intended to
catch people doing unsafe things and throw errors instead of letting
them crash the server.  Investigation reveals that they don't have
this effect because parallel workers were running their pre-commit
sequence with the checks disabled.  If they do something like try to
send notifications, it can lead to the worker getting an XID
assignment even though the master doesn't have one.  That's really
bad, and crashes the server.  That specific example should be
prohibited anyway (see patch #11) but even if we fix that I think this
is good tightening to prevent unpleasant surprises in the future.

0008-Invalidate-caches-after-cranking-up-a-parallel-worke.patch
invalidates invalidates system cache entries after cranking up a
parallel worker transaction.  This is needed here for the same reason
that the logical decoding code needs to do it after time traveling:
otherwise, the worker might have leftover entries in its caches as a
result of the startup transaction that are now bogus given the changes
in what it can see.

0009-Fix-a-problem-with-parallel-workers-being-unable-to-.patch fixes
a problem with workers being unable to precisely recreate the
authorization state as it existed in the parallel leader. They need to
do that, or else it's a security vulnerability.

0010-Prohibit-parallel-query-when-the-isolation-level-is-.patch
prohibits parallel query at the serializable isolation level.  This is
of course a restriction we'd rather not have, but it's a necessary one
for now because the predicate locking code doesn't understand the idea
of multiple processes with separate PGPROC structures being part of a
single transaction.

0011-Mark-more-functions-parallel-restricted-or-parallel-.patch marks
some functions as parallel-restricted or parallel-unsafe that in fact
are, but were not so marked by the commit that introduced the new
pg_proc flag.  This includes functions for sending notifications and a
few others.

0012-Rewrite-interaction-of-parallel-mode-with-parallel-e.patch
rejiggers the timing of enabling and disabling parallel mode when we
are attempting parallel execution.  The old coding turned out to be
fragile in multiple ways.  Since it's impractical to know at planning
time with ExecutorRun will be called with a non-zero tuple count, this
patch instead observes whether or not this happens, and if it does
happen, the parallel plan is forced to run serially.  In the old
coding, it instead just killed the parallel workers at the end of
ExecutorRun and therefore returned an incomplete result set.  There
might be some further rejiggering that could be done here that would
be even better than this, but I'm fairly certain this is better than
what we've got in the tree right now.

0013-Modify-tqueue-infrastructure-to-support-transient-re.patch
attempts to address a deficiency in the tqueue.c/tqueue.h machinery I
recently introduced: backends can have ephemeral record types for
which they use backend-local typmods that may not be the same between
the leader and the worker.  This patch has the worker send metadata
about the tuple descriptor for each such type, and the leader
registers the same tuple descriptor and then remaps the typmods from
the worker's typmod space to its own.  This seems to work, but I'm a
little concerned that there may be cases it doesn't cover.  Also,
there's room to question the overall approach.  The only other
alternative that springs readily to mind is to try to arrange things
during the planning phase so that we never try to pass records between
parallel backends in this way, but that seems like it would be hard to
code (and thus likely to have bugs) and also pretty limiting.

0014-Fix-problems-with-ParamListInfo-serialization-mechan.patch, which
I just posted on the Parallel Seq Scan thread as a standalone patch,
fixes pretty much what the name of the file suggests.  This actually
fixes two problems, one of which Noah spotted and commented on over on
that thread.  By pure coincidence, the last 'make check' regression
failure I was still troubleshooting needed a fix for that issue plus a
fix to plpgsql_param_fetch.  However, as I mentioned on the other
thread, I'm not quite sure which way to go with the change to
plpgsql_param_fetch so scrutiny of that point, in particular, would be
appreciated.  See also
http://www.postgresql.org/message-id/CA+TgmobN=wADVaUTwsH-xqvCdovkeRasuXw2k3R6vmpWig7raw@mail.gmail.com

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