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
Attachments:
v1-Allow-MATERIALIZED-VIEW-Rewrite-when-event-triggers.patchapplication/octet-stream; name=v1-Allow-MATERIALIZED-VIEW-Rewrite-when-event-triggers.patchDownload
From 405992de555a67ef91bc1c0f84e782c091bb00dd Mon Sep 17 00:00:00 2001
From: Onder Kalaci <onderkalaci@gmail.com>
Date: Tue, 9 Aug 2022 14:28:14 +0200
Subject: [PATCH] Allow MATERIALIZED VIEW Rewrite when event triggers exists
It looks like we forgot to allow table rewrite
on PG_CMDTAG. This commit fixes that.
---
src/include/tcop/cmdtaglist.h | 2 +-
src/test/regress/expected/create_am.out | 13 +++++++++++++
src/test/regress/sql/create_am.sql | 15 +++++++++++++++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/include/tcop/cmdtaglist.h b/src/include/tcop/cmdtaglist.h
index 2b1163ce33..9e94f44c5f 100644
--- a/src/include/tcop/cmdtaglist.h
+++ b/src/include/tcop/cmdtaglist.h
@@ -42,7 +42,7 @@ PG_CMDTAG(CMDTAG_ALTER_FUNCTION, "ALTER FUNCTION", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_INDEX, "ALTER INDEX", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_LANGUAGE, "ALTER LANGUAGE", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_LARGE_OBJECT, "ALTER LARGE OBJECT", true, false, false)
-PG_CMDTAG(CMDTAG_ALTER_MATERIALIZED_VIEW, "ALTER MATERIALIZED VIEW", true, false, false)
+PG_CMDTAG(CMDTAG_ALTER_MATERIALIZED_VIEW, "ALTER MATERIALIZED VIEW", true, true, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR, "ALTER OPERATOR", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR_CLASS, "ALTER OPERATOR CLASS", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR_FAMILY, "ALTER OPERATOR FAMILY", true, false, false)
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out
index e9a9283d7a..c292d4b4c0 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -271,6 +271,19 @@ SELECT amname FROM pg_class c, pg_am am
heap2
(1 row)
+-- ALTER MATERIALIZED VIEW should work with event triggers as well
+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();
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap;
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
+-- cleanup the trigger
+DROP FUNCTION empty_event_trigger() CASCADE;
+NOTICE: drop cascades to event trigger empty_triggger
SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heapmv;
count | count
-------+-------
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql
index 256884c959..ebed6a20f9 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -177,6 +177,21 @@ SELECT amname FROM pg_class c, pg_am am
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
SELECT amname FROM pg_class c, pg_am am
WHERE c.relam = am.oid AND c.oid = 'heapmv'::regclass;
+
+-- ALTER MATERIALIZED VIEW should work with event triggers as well
+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();
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap;
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
+
+-- cleanup the trigger
+DROP FUNCTION empty_event_trigger() CASCADE;
+
SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heapmv;
-- No support for multiple subcommands
ALTER TABLE heaptable SET ACCESS METHOD heap, SET ACCESS METHOD heap2;
--
2.34.1
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
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
Attachments:
v2-Allow-MATERIALIZED-VIEW-Rewrite-when-event-triggers.patchapplication/octet-stream; name=v2-Allow-MATERIALIZED-VIEW-Rewrite-when-event-triggers.patchDownload
From e5dac00c23e834463252e0c6b5669743c023ee40 Mon Sep 17 00:00:00 2001
From: Onder Kalaci <onderkalaci@gmail.com>
Date: Tue, 9 Aug 2022 14:28:14 +0200
Subject: [PATCH] Allow MATERIALIZED VIEW Rewrite when event triggers exists
It looks like we forgot to allow table rewrite
on PG_CMDTAG. This commit fixes that.
---
src/include/tcop/cmdtaglist.h | 2 +-
src/test/regress/expected/event_trigger.out | 9 +++++++++
src/test/regress/sql/event_trigger.sql | 12 ++++++++++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/src/include/tcop/cmdtaglist.h b/src/include/tcop/cmdtaglist.h
index 2b1163ce33..9e94f44c5f 100644
--- a/src/include/tcop/cmdtaglist.h
+++ b/src/include/tcop/cmdtaglist.h
@@ -42,7 +42,7 @@ PG_CMDTAG(CMDTAG_ALTER_FUNCTION, "ALTER FUNCTION", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_INDEX, "ALTER INDEX", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_LANGUAGE, "ALTER LANGUAGE", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_LARGE_OBJECT, "ALTER LARGE OBJECT", true, false, false)
-PG_CMDTAG(CMDTAG_ALTER_MATERIALIZED_VIEW, "ALTER MATERIALIZED VIEW", true, false, false)
+PG_CMDTAG(CMDTAG_ALTER_MATERIALIZED_VIEW, "ALTER MATERIALIZED VIEW", true, true, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR, "ALTER OPERATOR", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR_CLASS, "ALTER OPERATOR CLASS", true, false, false)
PG_CMDTAG(CMDTAG_ALTER_OPERATOR_FAMILY, "ALTER OPERATOR FAMILY", true, false, false)
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index c95c30b314..df8b5f4f2e 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -606,6 +606,15 @@ SELECT
start_rls_command | event trigger start_rls_command | event trigger | {start_rls_command} | {} | ("event trigger",,start_rls_command,start_rls_command)
(3 rows)
+-- Create a heap2 table am handler with heapam handler
+CREATE TABLE heaptable USING heap AS
+SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a;
+CREATE MATERIALIZED VIEW heapmv USING heap AS SELECT * FROM heaptable;
+-- we can rewrite materialized views when there are event triggers
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
+-- clean up objects created for testing rewrite mat. view when event triggers exist
+DROP MATERIALIZED VIEW heapmv;
+DROP TABLE heaptable;
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index 5e45e3f190..f2d656010c 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -463,6 +463,18 @@ SELECT
LATERAL pg_get_object_address(b.type, b.object_names, b.object_args) as a
ORDER BY e.evtname;
+-- Create a heap2 table am handler with heapam handler
+CREATE TABLE heaptable USING heap AS
+SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a;
+CREATE MATERIALIZED VIEW heapmv USING heap AS SELECT * FROM heaptable;
+
+-- we can rewrite materialized views when there are event triggers
+ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
+
+-- clean up objects created for testing rewrite mat. view when event triggers exist
+DROP MATERIALIZED VIEW heapmv;
+DROP TABLE heaptable;
+
DROP EVENT TRIGGER start_rls_command;
DROP EVENT TRIGGER end_rls_command;
DROP EVENT TRIGGER sql_drop_command;
--
2.34.1
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
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