Date: Tue, 27 Jun 2023 10:03:48 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18000: Access method used by matview can be dropped leaving
 broken matview
Message-ID: <20230627170348.GA1461334@nathanxps13>
References: <18000-9145c25b1af475ca@postgresql.org>
 <ZJlN4DFm4FvVxe6v@paquier.xyz>
 <ZJpqh3xoFtlwZS9u@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZJpqh3xoFtlwZS9u@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230627170348.GA1461334%40nathanxps13>
Precedence: bulk

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