From: | Stephen Frost <sfrost(at)snowman(dot)net> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: our checks for read-only queries are not great |
Date: | 2020-01-08 22:57:53 |
Message-ID: | 20200108225752.GM3195@tamriel.snowman.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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 (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 8, 2020 at 3:26 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > On Wed, Jan 8, 2020 at 2:57 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Stephen Frost | 2020-01-08 23:09:35 | Re: Removing pg_pltemplate and creating "trustable" extensions |
Previous Message | Peter Geoghegan | 2020-01-08 22:56:22 | Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index. |