Horton

Thread

Commits

  1. Allow event trigger table_rewrite for ALTER MATERIALIZED VIEW

  1. Materialized view rewrite is broken when there is an event trigger

    Önder Kalacı <onderkalaci@gmail.com> — 2022-08-09T12:55:06Z

    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

  2. Re: Materialized view rewrite is broken when there is an event trigger

    Michael Paquier <michael@paquier.xyz> — 2022-08-09T13:13:21Z

    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

  3. Re: Materialized view rewrite is broken when there is an event trigger

    Önder Kalacı <onderkalaci@gmail.com> — 2022-08-09T14:29:37Z

    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

  4. Re: Materialized view rewrite is broken when there is an event trigger

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-09T14:35:01Z

    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

  5. Re: Materialized view rewrite is broken when there is an event trigger

    Michael Paquier <michael@paquier.xyz> — 2022-08-17T05:56:25Z

    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

  6. Re: Materialized view rewrite is broken when there is an event trigger

    Michael Paquier <michael@paquier.xyz> — 2022-08-17T06:00:20Z

    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