Thread: Materialized view rewrite is broken when there is an event trigger
Hi,
Attached a patch to fix as well. If the patch looks good to you, can you consider getting this to PG 15?
Steps to repro:
-- some basic examples from src/test/regress/sql/create_am.sql
CREATE TABLE heaptable USING heap AS
SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a;
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
CREATE MATERIALIZED VIEW heapmv USING heap AS SELECT * FROM heaptable;
-- altering MATERIALIZED
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap;
-- setup event trigger
CREATE OR REPLACE FUNCTION empty_event_trigger()
RETURNS event_trigger AS $$
DECLARE
BEGIN
END;
$$ LANGUAGE plpgsql;
CREATE EVENT TRIGGER empty_triggger ON sql_drop EXECUTE PROCEDURE empty_event_trigger();
-- now, after creating an event trigger, ALTER MATERIALIZED VIEW fails unexpectedly
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
ERROR: unexpected command tag "ALTER MATERIALIZED VIEW"
Thanks,
Onder Kalaci
Attachment
On Tue, Aug 09, 2022 at 02:55:06PM +0200, Önder Kalacı wrote: > Attached a patch to fix as well. If the patch looks good to you, can you > consider getting this to PG 15? Thanks, this one is on me so I have added an open item. I will unfortunately not be able to address that this week because of life, but I should be able to grab a little bit of time next week to look at what you have. Please note that we should not add an event in create_am.sql even if it is empty, as it gets run in parallel of other tests so there could be interferences. I think that this had better live in sql/event_trigger.sql, even if it requires an extra table AM to check this specific case. -- Michael
Attachment
Hi,
I should be able to grab a little bit of time next week to look at
what you have.
Thanks!
Please note that we should not add an event in create_am.sql even if
it is empty, as it gets run in parallel of other tests so there could
be interferences. I think that this had better live in
sql/event_trigger.sql, even if it requires an extra table AM to check
this specific case.
--
Moved the test to event_trigger.sql.
> parallel group (2 tests, in groups of 1): event_trigger oidjoins
Though, it also seems to run in parallel, but I assume the parallel test already works fine with concurrent event triggers?
Thanks,
Onder
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> Please note that we should not add an event in create_am.sql even if
> it is empty, as it gets run in parallel of other tests so there could
> be interferences. I think that this had better live in
> sql/event_trigger.sql, even if it requires an extra table AM to check
> this specific case.
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.
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?
regards, tom lane
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
Attachment
On Tue, Aug 09, 2022 at 04:29:37PM +0200, Önder Kalacı wrote: > Though, it also seems to run in parallel, but I assume the parallel test > already works fine with concurrent event triggers? We've had issues with such assumptions in the past as event triggers are global, see for example 676858b or c219cbf, so I would rather avoid more of that. -- Michael