our checks for read-only queries are not great

I spent some time studying the question of how we classify queries as
either read-only or not, and our various definitions of read-only, and
found some bugs. Specifically:

1. check_xact_readonly() prohibits most kinds of DDL in read-only
transactions, but a bunch of recently-added commands were not added to
the list. The missing node types are T_CreatePolicyStmt,
T_AlterPolicyStmt, T_CreateAmStmt, T_CreateStatsStmt,
T_AlterStatsStmt, and T_AlterCollationStmt, which means you can run
these commands in a read-only transaction with no problem and even
attempt to run them on a standby. The ones I tested on a standby all
fail with random-ish error messages due to lower-level checks, but
that's not a great situation.

2. There are comments in utility.c which assert that certain commands
are "forbidden in parallel mode due to CommandIsReadOnly." That's
technically correct, but silly and misleading.These commands wouldn't
be running in parallel mode unless they were running inside of a
function or procedure or something, and, if they are,
CommandIsReadOnly() checks in spi.c or functions.c would prevent not
only these commands but, in fact, all utility commands, so calling out
those particular ones is just adding confusion. Also, the underlying
restriction is unnecessary, because there's no good reason to prevent
the use of things like SHOW and DO in parallel mode, yet we currently
do.

The problems mentioned under (1) are technically the fault of the
people who wrote, reviewed, and committed the patches which added
those command types, and the problems mentioned under (2) are
basically my fault, dating back to the original ParallelContext patch.
However, I think that all of them can be tracked back to a more
fundamental underlying cause, which is that the way that the various
restrictions on read-write queries are implemented is pretty
confusing. Attached is a patch I wrote to try to improve things. It
centralizes three decisions that are currently made in different
places in a single place: (a) can this be run in a read only
transaction? (b) can it run in parallel mode? (c) can it run on a
standby? -- and along the way, it fixes the problems mentioned above
and tries to supply slightly improved comments. Perhaps we should
back-patch fixes at least for (1) even if this gets committed, but I
guess my first question is what people think of this approach to the
problem.

Thanks,

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