Thread: BUG #18000: Access method used by matview can be dropped leaving broken matview
BUG #18000: Access method used by matview can be dropped leaving broken matview
From
PG Bug reporting form
Date:
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.
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Nathan Bossart
Date:
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
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Andres Freund
Date:
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
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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
Attachment
Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
From
Michael Paquier
Date:
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