Greetings, (I'm also overall in favor of the direction this is going, so general +1 from me, and I took a quick look through the patch and didn't particularly see anything I didn't like besides what's commented on below.)
* Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Jan 8, 2020 at 3:26 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Robert Haas <robertmh...@gmail.com> writes: > > > On Wed, Jan 8, 2020 at 2:57 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > >> * I find COMMAND_IS_WEAKLY_READ_ONLY to be a more confusing concept > > >> than what it replaces. The test for LockStmt is an example --- the > > >> comment talks about restricting locks during recovery, which is fine and > > >> understandable, but then it's completely unobvious that the actual code > > >> implements that behavior rather than some other one. > > > > > Uh, suggestions? > > > > COMMAND_NOT_IN_RECOVERY, maybe? > > Well, maybe I should just get rid of COMMAND_IS_WEAKLY_READ_ONLY and > return individual COMMAND_OK_IN_* flags for those cases. Yeah, I don't like the WEAKLY_READ_ONLY idea either- explicitly having COMMAND_OK_IN_X seems cleaner. > > >> * ALTER SYSTEM SET is readonly? Say what? > > > > > It would be extremely lame and a huge usability regression to > > > arbitrary restrict ALTER SYSTEM SET on standby nodes for no reason. > > > > I didn't say that it shouldn't be allowed on standby nodes. > > Oh, OK. I guess I misunderstood. I agree that we want ALTER SYSTEM SET to work on standbys, but it seems there isn't really disagreement there. > > I said > > it shouldn't be allowed in transactions that have explicitly declared > > themselves to be read-only. Maybe we need to disaggregate those > > concepts a bit more --- a refactoring such as this would be a fine > > time to do that. > > Yeah, the current rules are pretty weird. Aside from ALTER SYSTEM .. > SET, read-only transaction seem to allow writes to temporary relations > and sequences, plus CLUSTER, REINDEX, VACUUM, PREPARE, ROLLBACK > PREPARED, and COMMIT PREPARED, all of which sound a lot like writes to > me. They also allow LISTEN and SET which are have transactional > behavior in general but for some reason don't feel they need to > respect the R/O property. I worry that if we start whacking these > behaviors around we'll get complaints, so I'm cautious about doing > that. At the least, we would need to have a real clear definition, and > if there is such a definition that covers the current cases, I can't > guess what it is. Forget ALTER SYSTEM for a minute -- why is it OK to > rewrite a table via CLUSTER in a R/O transaction, but not OK to do an > ALTER TABLE that changes the clustering index? Why is it not OK to > LISTEN on a standby (and presumably not get any notifications until a > promotion occurs) but OK to UNLISTEN? Whatever reasons may have > justified the current choice of behaviors are probably lost in the > sands of time; they are for sure unknown to me. That a 'read-only' transaction can call CLUSTER is definitely bizarre to me. As relates to 'UN-SOMETHING', having those be allowed makes sense, to me anyway, since connection poolers like to do those things and it should be a no-op more-or-less by definition. SET isn't changing data blocks, so that also seems ok for a read-only transaction.. but, yeah, there's no real great hard-and-fast-rule we've been following. Would we be able to make a rule of "can't change on-disk stuff, except for things in temporary schemas" and have it stick without a lot of complaints? Seems like that would address Tom's ALTER SYSTEM SET concern, and would mean CLUSTER/REINDEX/VACUUM are disallowed in a backwards-incompatible way (though I think I'm fine with that..), and SET would still be allowed (which strikes me as correct too). I'm not quite sure how I feel about LISTEN, but that it could possibly actually be used post-promotion and doesn't change on-disk stuff makes me feel like it actually probably should be allowed. Just looking at what was mentioned here- if there's other cases where this idea falls flat then let's discuss them. Thanks, Stephen
signature.asc
Description: PGP signature
