On Tue, Aug 09, 2022 at 10:35:01AM -0400, Tom Lane wrote:
> Agreed this is a bug, but I do not think we should add the proposed
> regression test, regardless of where exactly. It looks like expending
> a lot of cycles forevermore to watch for an extremely unlikely thing,
> ie that we break this for ALTER MATERIALIZED VIEW and not anything
> else.
Hmm. I see a second point in keeping a test in this area, because we
have nothing that directly checks after AT_REWRITE_ACCESS_METHOD as
introduced by b048326. It makes me wonder whether we should have a
second test for a plain table with SET ACCESS METHOD, actually, but
we have already cases for rewrites there, so..
> I think the real problem here is that we don't have any mechanism
> for verifying that table_rewrite_ok is correct. The "cross-check"
> in EventTriggerCommonSetup is utterly worthless, as this failure
> shows. Does it give any confidence at all that there are no other
> mislabelings? I sure have none now. What can we do to verify that
> more rigorously? Or maybe we should find a way to get rid of the
> table_rewrite_ok flag altogether?
This comes down to the dependency between the event trigger paths in
utility.c and tablecmds.c, which gets rather trickier with the ALTERs
on various relkinds. I don't really know about if we could cut this
specific flag, perhaps we could manage a list of command tags
supported for it as that's rather short. I can also see that
something could be done for the firing matrix in the docs, as well
(the proposed patch has forgotten the docs). That's not something
that should be done for v15 anyway, so I have fixed the issue at hand
to take care of this open item.
--
Michael
