Re: BUG #18000: Access method used by matview can be dropped leaving broken matview

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