Обсуждение: BUG #18000: Access method used by matview can be dropped leaving broken matview

The following bug has been logged on the website:

Bug reference:      18000
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 16beta1
Operating system:   Ubuntu 22.04
Description:

The following script:
BEGIN;
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
CREATE MATERIALIZED VIEW heapmv USING heap AS SELECT 1::int;
ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap2;
DROP ACCESS METHOD heap2;
SELECT * FROM heapmv;
COMMIT;
SELECT * FROM heapmv;
DROP MATERIALIZED VIEW heapmv;

drops the access method heap2, but leaves the materialized view heapmv,
which then can not be accessed nor dropped:
ERROR:  cache lookup failed for access method 16390

You cannot dump the database, too:
pg_dump: error: query failed: ERROR:  cache lookup failed for access method
16390
pg_dump: detail: Query was: SELECT
pg_catalog.pg_get_viewdef('16391'::pg_catalog.oid) AS viewdef

But if the access method was set during a matview creation, it's deletion
is
prevented as expected:
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
CREATE MATERIALIZED VIEW heapmv USING heap2 AS SELECT 1::int;
DROP ACCESS METHOD heap2;
ERROR:  cannot drop access method heap2 because other objects depend on it
DETAIL:  materialized view heapmv depends on access method heap2
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

Reproduced on REL_15_STABLE .. master.

On Mon, Jun 26, 2023 at 08:00:01AM +0000, PG Bug reporting form wrote:
> But if the access method was set during a matview creation, it's deletion
> is
> prevented as expected:
> CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
> CREATE MATERIALIZED VIEW heapmv USING heap2 AS SELECT 1::int;
> DROP ACCESS METHOD heap2;
> ERROR:  cannot drop access method heap2 because other objects depend on it
> DETAIL:  materialized view heapmv depends on access method heap2
> HINT:  Use DROP ... CASCADE to drop the dependent objects too.
>
> Reproduced on REL_15_STABLE .. master.

Thanks for the report!

ALTER TABLE .. SET ACCESS METHOD is on me.  Will look..
--
Michael

On Mon, Jun 26, 2023 at 05:35:44PM +0900, Michael Paquier wrote:
> Thanks for the report!
>
> ALTER TABLE .. SET ACCESS METHOD is on me.  Will look..

It took me some time to find out what is going on here, but the root
of the issue comes down to when the relation is rewritten when its
access method is updated.

ATRewriteTables() uses make_new_heap() to build the new, rewritten
relfilenode, and creates a temporary relation that includes the
correct entries in pg_depend, which would be one on the namespace
where the relation is located and a second one for the table AM (if
the AM is not a pinned reference).  After that comes the relfilenode
swap, that switches the old and new relfile nodes, and drops the
temporary heap relation created by make_new_heap().  In all this
flow, there is nothing that registers a new dependency to the AM if
it has been changed by ALTER TABLE on the *old* relation that remains
around.

In order to do something about that, I think that we should just do
something similar to schema changes where we switch the dependency
with a changeDependencyFor(), and that the best spot is
swap_relation_files(), where the pg_class tuple of the old relation
has its persistence, tablespace and AM updated according to the
rewrite.  One reason is that there were discussions where it could
make sense to update a table AM on CLUSTER or VACUUM, for example, so
that would cover this spot, but that's only hypothetical currently.

The path of a relation rewrite cares also about tablespaces, however
only shared dependency entries for relations without storage matter,
so as a tablespace is not dropped when it should not.  That's where
SetRelationTableSpace() does this work.

(Note that a patch has been recently proposed to add AMs to
partitioned tables, which I think is also impacted by the same issue,
and we would need to be more careful in this second case, but that's
work for 17~).

Attached is a patch to take care of the issue, with regression tests
looking at pg_depend to make sure that the correct entries are
created or removed.

Thoughts?
--
Michael

On Tue, Jun 27, 2023 at 01:50:15PM +0900, Michael Paquier wrote:
> Attached is a patch to take care of the issue, with regression tests
> looking at pg_depend to make sure that the correct entries are
> created or removed.

A diff got eaten in this process (see pgstat_progress_update_param,
not sure what happened), so attached is a v2 to correct the shot.

Sorry for the extra noise.
--
Michael

On Tue, Jun 27, 2023 at 01:50:15PM +0900, Michael Paquier wrote:
> In order to do something about that, I think that we should just do
> something similar to schema changes where we switch the dependency
> with a changeDependencyFor(), and that the best spot is
> swap_relation_files(), where the pg_class tuple of the old relation
> has its persistence, tablespace and AM updated according to the
> rewrite.  One reason is that there were discussions where it could
> make sense to update a table AM on CLUSTER or VACUUM, for example, so
> that would cover this spot, but that's only hypothetical currently.

Agreed.

> The path of a relation rewrite cares also about tablespaces, however
> only shared dependency entries for relations without storage matter,
> so as a tablespace is not dropped when it should not.  That's where
> SetRelationTableSpace() does this work.

I looked into the tablespace path, too.  Since materialized views have
storage, we don't have to worry about ALTER MATERIALIZED VIEW SET
TABLESPACE losing the dependency records (that IIUC won't exist anyway), as
dropping the tablespace will fail because it won't be empty.  This feels
kind of fragile, but I don't have a better proposal at the moment.

> (Note that a patch has been recently proposed to add AMs to
> partitioned tables, which I think is also impacted by the same issue,
> and we would need to be more careful in this second case, but that's
> work for 17~).

I haven't looked at the patch, but assuming it's not calling
finish_heap_swap(), it might be okay.  But it does seem prudent to check
that the dependency records are handled correctly.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Hi,

On 2023-06-27 15:33:30 +0900, Michael Paquier wrote:
> +    /*
> +     * Now that pg_class has been updated with its relevant information for
> +     * the swap, update the dependency of the relation to point to its new
> +     * table AM, if it has changed.
> +     */
> +    if (relam1 != relam2)
> +    {
> +        changeDependencyFor(RelationRelationId,
> +                            r1,
> +                            AccessMethodRelationId,
> +                            relam1,
> +                            relam2);
> +    }

Should we check changeDependencyFor()'s return value? On a first glance it
looks like it would be an error to return 0 in this case?

Greetings,

Andres Freund

On Tue, Jun 27, 2023 at 11:23:32AM -0700, Andres Freund wrote:
> Should we check changeDependencyFor()'s return value? On a first glance it
> looks like it would be an error to return 0 in this case?

Agreed.  I was wondering if this was worth having in this path (other
code paths are rather lax about that), but yes let's do it as there
should not be more than one record in pg_depend.
--
Michael

On Tue, Jun 27, 2023 at 10:03:48AM -0700, Nathan Bossart wrote:
> I haven't looked at the patch, but assuming it's not calling
> finish_heap_swap(), it might be okay.  But it does seem prudent to check
> that the dependency records are handled correctly.

Double-checking, the patch is able to handle that with an extra
changeDependencyFor() in a code path for relkinds that have no
storage.  What the patch does not do it look at pg_depend to see if
the dependency records are stored correctly, and that's something it
had better do.
--
Michael

On Wed, Jun 28, 2023 at 08:11:51AM +0900, Michael Paquier wrote:
> Agreed.  I was wondering if this was worth having in this path (other
> code paths are rather lax about that), but yes let's do it as there
> should not be more than one record in pg_depend.

There was something that was itching me a little bit here: shouldn't
we also switch the AM dependency of the second relation?  In the
context of a table rewrite, the new relation is temporary and quickly
dropped after the relation files are swapped, but that may not be the
case of all the callers of swap_relation_files() depending on the
persistency of what they swap and what they want to do.

Thoughts?
--
Michael

On Thu, Jun 29, 2023 at 07:57:40AM +0900, Michael Paquier wrote:
> There was something that was itching me a little bit here: shouldn't
> we also switch the AM dependency of the second relation?  In the
> context of a table rewrite, the new relation is temporary and quickly
> dropped after the relation files are swapped, but that may not be the
> case of all the callers of swap_relation_files() depending on the
> persistency of what they swap and what they want to do.

I have forgotten to update this thread.  This has been fixed with
97d8910, where the pg_depend entries on AMof both relations, r1 and
r2, are updated during the swap.
--
Michael