Re: our checks for read-only queries are not great

Stephen Frost <[email protected]> Wed, 8 Jan 2020 17:57:53 -0500
Newsgroups	gmane.comp.db.postgresql.devel.general
Message-ID	<[email protected]>
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 ([email protected]) wrote:
> On Wed, Jan 8, 2020 at 3:26 PM Tom Lane <[email protected]> wrote:
> > Robert Haas <[email protected]> writes:
> > > On Wed, Jan 8, 2020 at 2:57 PM Tom Lane <[email protected]> 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 (application/pgp-signature, 819 B)