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
0001-Fix-problems-with-read-only-query-checks-and-refacto.patch
Description: Binary data
