pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
BUG #18000: Access method used by matview can be dropped leaving broken matview
10+ messages / 4 participants
[nested] [flat]

Commits from this thread:
  2023-06-29 97d8910104 93401ec02f  Fix pg_depend entry to AMs after ALTER TABLE .. SET ACCESS METHOD
* BUG #18000: Access method used by matview can be dropped leaving broken matview
@ 2023-06-26 08:00 PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 10+ messages in thread

From: PG Bug reporting form @ 2023-06-26 08:00 UTC (permalink / raw)
  To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com

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.



^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
@ 2023-06-26 08:35 ` Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 10+ messages in thread

From: Michael Paquier @ 2023-06-26 08:35 UTC (permalink / raw)
  To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-06-27 04:50   ` Michael Paquier <michael@paquier.xyz>
  2023-06-27 06:33     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 17:03     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Nathan Bossart <nathandbossart@gmail.com>
  0 siblings, 2 replies; 10+ messages in thread

From: Michael Paquier @ 2023-06-27 04:50 UTC (permalink / raw)
  To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

v1-0001-Fix-dependencies-under-ALTER-TABLE-SET-ACCESS-MET.patch (text/x-diff, 5.1K) download From 19c93986d941c3e7749ce4b56b7465cc8f64886b Mon Sep 17 00:00:00 2001 From: Michael Paquier <michael@paquier.xyz> Date: Tue, 27 Jun 2023 13:48:51 +0900 Subject: [PATCH v1] Fix dependencies under ALTER TABLE SET ACCESS METHOD --- src/backend/commands/cluster.c | 22 +++++++++++++++---- src/test/regress/expected/create_am.out | 29 +++++++++++++++++++++++++ src/test/regress/sql/create_am.sql | 18 +++++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3bfabb6d10..032e6af602 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1070,6 +1070,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber2; RelFileNumber swaptemp; char swptmpchr; + Oid relam1, + relam2; /* We need writable copies of both pg_class tuples. */ relRelation = table_open(RelationRelationId, RowExclusiveLock); @@ -1086,6 +1088,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber1 = relform1->relfilenode; relfilenumber2 = relform2->relfilenode; + relam1 = relform1->relam; + relam2 = relform2->relam; if (RelFileNumberIsValid(relfilenumber1) && RelFileNumberIsValid(relfilenumber2)) @@ -1257,6 +1261,20 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, CacheInvalidateRelcacheByTuple(reltup2); } + /* + * 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); + } + /* * Post alter hook for modified relations. The change to r2 is always * internal, but r1 depends on the invocation context. @@ -1493,10 +1511,6 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap, reindex_relation(OIDOldHeap, reindex_flags, &reindex_params); - /* Report that we are now doing clean up */ - pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE, - PROGRESS_CLUSTER_PHASE_FINAL_CLEANUP); - /* * If the relation being rebuilt is pg_class, swap_relation_files() * couldn't update pg_class's own pg_class entry (check comments in diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out index e9a9283d7a..4eec292e65 100644 --- a/src/test/regress/expected/create_am.out +++ b/src/test/regress/expected/create_am.out @@ -240,6 +240,35 @@ SELECT amname FROM pg_class c, pg_am am heap (1 row) +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------------+--------- + table heaptable | access method heap2 | n + table heaptable | schema public | n +(2 rows) + +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------+--------- + table heaptable | schema public | n +(1 row) + ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql index 256884c959..f1c809b8e0 100644 --- a/src/test/regress/sql/create_am.sql +++ b/src/test/regress/sql/create_am.sql @@ -166,6 +166,24 @@ CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; -- 2.40.1
signature.asc (application/pgp-signature, 832B) download ^ permalink raw reply [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-06-27 06:33     ` Michael Paquier <michael@paquier.xyz>
  2023-06-27 18:23       ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Andres Freund <andres@anarazel.de>
  1 sibling, 1 reply; 10+ messages in thread

From: Michael Paquier @ 2023-06-27 06:33 UTC (permalink / raw)
  To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

v2-0001-Fix-dependencies-under-ALTER-TABLE-SET-ACCESS-MET.patch (text/x-diff, 4.7K) download From ab7ad308dfbf3d4587d88b5133b44c201b4569e2 Mon Sep 17 00:00:00 2001 From: Michael Paquier <michael@paquier.xyz> Date: Tue, 27 Jun 2023 15:32:42 +0900 Subject: [PATCH v2] Fix dependencies under ALTER TABLE SET ACCESS METHOD --- src/backend/commands/cluster.c | 18 +++++++++++++++ src/test/regress/expected/create_am.out | 29 +++++++++++++++++++++++++ src/test/regress/sql/create_am.sql | 18 +++++++++++++++ 3 files changed, 65 insertions(+) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3bfabb6d10..88432a9518 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1070,6 +1070,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber2; RelFileNumber swaptemp; char swptmpchr; + Oid relam1, + relam2; /* We need writable copies of both pg_class tuples. */ relRelation = table_open(RelationRelationId, RowExclusiveLock); @@ -1086,6 +1088,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber1 = relform1->relfilenode; relfilenumber2 = relform2->relfilenode; + relam1 = relform1->relam; + relam2 = relform2->relam; if (RelFileNumberIsValid(relfilenumber1) && RelFileNumberIsValid(relfilenumber2)) @@ -1257,6 +1261,20 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, CacheInvalidateRelcacheByTuple(reltup2); } + /* + * 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); + } + /* * Post alter hook for modified relations. The change to r2 is always * internal, but r1 depends on the invocation context. diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out index e9a9283d7a..4eec292e65 100644 --- a/src/test/regress/expected/create_am.out +++ b/src/test/regress/expected/create_am.out @@ -240,6 +240,35 @@ SELECT amname FROM pg_class c, pg_am am heap (1 row) +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------------+--------- + table heaptable | access method heap2 | n + table heaptable | schema public | n +(2 rows) + +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------+--------- + table heaptable | schema public | n +(1 row) + ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql index 256884c959..f1c809b8e0 100644 --- a/src/test/regress/sql/create_am.sql +++ b/src/test/regress/sql/create_am.sql @@ -166,6 +166,24 @@ CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; -- 2.40.1
signature.asc (application/pgp-signature, 832B) download ^ permalink raw reply [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 06:33     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-06-27 18:23       ` Andres Freund <andres@anarazel.de>
  2023-06-27 23:11         ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 10+ messages in thread

From: Andres Freund @ 2023-06-27 18:23 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 06:33     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 18:23       ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Andres Freund <andres@anarazel.de>
@ 2023-06-27 23:11         ` Michael Paquier <michael@paquier.xyz>
  2023-06-28 22:57           ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 10+ messages in thread

From: Michael Paquier @ 2023-06-27 23:11 UTC (permalink / raw)
  To: Andres Freund <andres@anarazel.de>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

v2-0001-Fix-dependencies-under-ALTER-TABLE-SET-ACCESS-MET.patch (text/x-diff, 4.8K) download From 6ef48042e1eefd30b16438c03e584b41733b3e49 Mon Sep 17 00:00:00 2001 From: Michael Paquier <michael@paquier.xyz> Date: Wed, 28 Jun 2023 08:06:31 +0900 Subject: [PATCH v2] Fix dependencies under ALTER TABLE SET ACCESS METHOD --- src/backend/commands/cluster.c | 21 ++++++++++++++++++ src/test/regress/expected/create_am.out | 29 +++++++++++++++++++++++++ src/test/regress/sql/create_am.sql | 18 +++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3bfabb6d10..e84460a43e 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1070,6 +1070,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber2; RelFileNumber swaptemp; char swptmpchr; + Oid relam1, + relam2; /* We need writable copies of both pg_class tuples. */ relRelation = table_open(RelationRelationId, RowExclusiveLock); @@ -1086,6 +1088,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber1 = relform1->relfilenode; relfilenumber2 = relform2->relfilenode; + relam1 = relform1->relam; + relam2 = relform2->relam; if (RelFileNumberIsValid(relfilenumber1) && RelFileNumberIsValid(relfilenumber2)) @@ -1257,6 +1261,23 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, CacheInvalidateRelcacheByTuple(reltup2); } + /* + * 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) + { + if (changeDependencyFor(RelationRelationId, + r1, + AccessMethodRelationId, + relam1, + relam2) != 1) + elog(ERROR, "failed to change access method dependency for relation \"%s.%s\"", + get_namespace_name(get_rel_namespace(r1)), + get_rel_name(r1)); + } + /* * Post alter hook for modified relations. The change to r2 is always * internal, but r1 depends on the invocation context. diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out index e9a9283d7a..4eec292e65 100644 --- a/src/test/regress/expected/create_am.out +++ b/src/test/regress/expected/create_am.out @@ -240,6 +240,35 @@ SELECT amname FROM pg_class c, pg_am am heap (1 row) +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------------+--------- + table heaptable | access method heap2 | n + table heaptable | schema public | n +(2 rows) + +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------+--------- + table heaptable | schema public | n +(1 row) + ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql index 256884c959..f1c809b8e0 100644 --- a/src/test/regress/sql/create_am.sql +++ b/src/test/regress/sql/create_am.sql @@ -166,6 +166,24 @@ CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid,refobjid,refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; -- 2.40.1
signature.asc (application/pgp-signature, 832B) download ^ permalink raw reply [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 06:33     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 18:23       ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Andres Freund <andres@anarazel.de>
  2023-06-27 23:11         ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-06-28 22:57           ` Michael Paquier <michael@paquier.xyz>
  2023-07-03 02:26             ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  0 siblings, 1 reply; 10+ messages in thread

From: Michael Paquier @ 2023-06-28 22:57 UTC (permalink / raw)
  To: Andres Freund <andres@anarazel.de>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

v3-0001-Fix-pg_depend-entry-to-AMs-after-ALTER-TABLE-.-SE.patch (text/x-diff, 9.3K) download From 39ddc35db15222a7b5d6ee94fa27023e12112ae9 Mon Sep 17 00:00:00 2001 From: Michael Paquier <michael@paquier.xyz> Date: Thu, 29 Jun 2023 07:53:16 +0900 Subject: [PATCH v3] Fix pg_depend entry to AMs after ALTER TABLE .. SET ACCESS METHOD ALTER TABLE .. SET ACCESS METHOD was not registering a dependency to the new access method with the relation altered in its rewrite phase, making possible the drop of an access method even if there are relations that depend on it. During the rewrite, a temporary relation is created to build the new relation files before swapping the new and old files, and, while the temporary relation was registering a correct dependency to the new AM, the old relation did not do that. A dependency on the access method is added when the relation files are swapped, which is the point where pg_class is updated with the new access method. Materialized views and tables use the same code path, hence both were impacted. Backpatch down to 15, where this command has been introduced. Reported-by: Alexander Lakhin Reviewed-by: Nathan Bossart, Andres Freund Discussion: https://postgr.es/m/18000-9145c25b1af475ca@postgresql.org Backpatch-through: 15 --- src/backend/commands/cluster.c | 29 +++++++++++++++++++++++ src/bin/pg_waldump/pg_waldump.c | 3 ++- src/bin/pg_waldump/t/002_save_fullpage.pl | 9 +++---- src/test/regress/expected/create_am.out | 29 +++++++++++++++++++++++ src/test/regress/sql/create_am.sql | 18 ++++++++++++++ doc/src/sgml/ref/pg_waldump.sgml | 9 ++++++- 6 files changed, 91 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c index 3bfabb6d10..03b24ab90f 100644 --- a/src/backend/commands/cluster.c +++ b/src/backend/commands/cluster.c @@ -1070,6 +1070,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber2; RelFileNumber swaptemp; char swptmpchr; + Oid relam1, + relam2; /* We need writable copies of both pg_class tuples. */ relRelation = table_open(RelationRelationId, RowExclusiveLock); @@ -1086,6 +1088,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, relfilenumber1 = relform1->relfilenode; relfilenumber2 = relform2->relfilenode; + relam1 = relform1->relam; + relam2 = relform2->relam; if (RelFileNumberIsValid(relfilenumber1) && RelFileNumberIsValid(relfilenumber2)) @@ -1257,6 +1261,31 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg_class, CacheInvalidateRelcacheByTuple(reltup2); } + /* + * Now that pg_class has been updated with its relevant information for + * the swap, update the dependency of the relations to point to their new + * table AM, if it has changed. + */ + if (relam1 != relam2) + { + if (changeDependencyFor(RelationRelationId, + r1, + AccessMethodRelationId, + relam1, + relam2) != 1) + elog(ERROR, "failed to change access method dependency for relation \"%s.%s\"", + get_namespace_name(get_rel_namespace(r1)), + get_rel_name(r1)); + if (changeDependencyFor(RelationRelationId, + r2, + AccessMethodRelationId, + relam2, + relam1) != 1) + elog(ERROR, "failed to change access method dependency for relation \"%s.%s\"", + get_namespace_name(get_rel_namespace(r2)), + get_rel_name(r2)); + } + /* * Post alter hook for modified relations. The change to r2 is always * internal, but r1 depends on the invocation context. diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index c6d3ae6344..96845e1a1a 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -518,7 +518,8 @@ XLogRecordSaveFPWs(XLogReaderState *record, const char *savepath) else pg_fatal("invalid fork number: %u", fork); - snprintf(filename, MAXPGPATH, "%s/%08X-%08X.%u.%u.%u.%u%s", savepath, + snprintf(filename, MAXPGPATH, "%s/%08X-%08X-%08X.%u.%u.%u.%u%s", savepath, + record->seg.ws_tli, LSN_FORMAT_ARGS(record->ReadRecPtr), rnode.spcOid, rnode.dbOid, rnode.relNumber, blk, forkname); diff --git a/src/bin/pg_waldump/t/002_save_fullpage.pl b/src/bin/pg_waldump/t/002_save_fullpage.pl index 831ffdefef..f0725805f2 100644 --- a/src/bin/pg_waldump/t/002_save_fullpage.pl +++ b/src/bin/pg_waldump/t/002_save_fullpage.pl @@ -79,15 +79,16 @@ $node->command_ok( 'pg_waldump with --save-fullpage runs'); # This regexp will match filenames formatted as: -# XXXXXXXX-XXXXXXXX.DBOID.TLOID.NODEOID.dd_fork with the components being: -# - WAL LSN in hex format, -# - Tablespace OID (0 for global) +# TLI-LSNh-LSNl.TBLSPCOID.DBOID.NODEOID.dd_fork with the components being: +# - Timeline ID in hex format. +# - WAL LSN in hex format, as two 8-character numbers. +# - Tablespace OID (0 for global). # - Database OID. # - Relfilenode. # - Block number. # - Fork this block came from (vm, init, fsm, or main). my $file_re = - qr/^([0-9A-F]{8})-([0-9A-F]{8})[.][0-9]+[.][0-9]+[.][0-9]+[.][0-9]+(?:_vm|_init|_fsm|_main)?$/; + qr/^[0-9A-F]{8}-([0-9A-F]{8})-([0-9A-F]{8})[.][0-9]+[.][0-9]+[.][0-9]+[.][0-9]+(?:_vm|_init|_fsm|_main)?$/; my $file_count = 0; diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/expected/create_am.out index e9a9283d7a..b50293d514 100644 --- a/src/test/regress/expected/create_am.out +++ b/src/test/regress/expected/create_am.out @@ -240,6 +240,35 @@ SELECT amname FROM pg_class c, pg_am am heap (1 row) +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid, refobjid, refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------------+--------- + table heaptable | access method heap2 | n + table heaptable | schema public | n +(2 rows) + +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid, refobjid, refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; + obj | objref | deptype +-----------------+---------------+--------- + table heaptable | schema public | n +(1 row) + ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/create_am.sql index 256884c959..2785ffd8bb 100644 --- a/src/test/regress/sql/create_am.sql +++ b/src/test/regress/sql/create_am.sql @@ -166,6 +166,24 @@ CREATE TABLE heaptable USING heap AS SELECT a, repeat(a::text, 100) FROM generate_series(1,9) AS a; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; +-- Switching to heap2 adds new dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap2; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid, refobjid, refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; +-- Switching to heap should not have a dependency entry to the AM. +ALTER TABLE heaptable SET ACCESS METHOD heap; +SELECT pg_describe_object(classid, objid, objsubid) as obj, + pg_describe_object(refclassid, refobjid, refobjsubid) as objref, + deptype + FROM pg_depend + WHERE classid = 'pg_class'::regclass AND + objid = 'heaptable'::regclass + ORDER BY 1, 2; ALTER TABLE heaptable SET ACCESS METHOD heap2; SELECT amname FROM pg_class c, pg_am am WHERE c.relam = am.oid AND c.oid = 'heaptable'::regclass; diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index e5f9637847..4592d6016a 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -283,7 +283,7 @@ PostgreSQL documentation </para> <para> The full page images are saved with the following file name format: - <literal><replaceable>LSN</replaceable>.<replaceable>RELTABLESPACE</replaceable>.<replaceable>DATOID</replaceable>.<replaceable>RELNODE</replaceable>.<replaceable>BLKNO</replaceable><replaceable>FORK</replaceable></literal> + <literal><replaceable>TIMELINE</replaceable>-<replaceable>LSN</replaceable>.<replaceable>RELTABLESPACE</replaceable>.<replaceable>DATOID</replaceable>.<replaceable>RELNODE</replaceable>.<replaceable>BLKNO</replaceable><replaceable>FORK</replaceable></literal> The file names are composed of the following parts: <informaltable> @@ -296,6 +296,13 @@ PostgreSQL documentation </thead> <tbody> + <row> + <entry>TIMELINE</entry> + <entry>The timeline of the WAL segment file where the record + is located formatted as one 8-character hexadecimal number + <literal>%08X</literal></entry> + </row> + <row> <entry>LSN</entry> <entry>The <acronym>LSN</acronym> of the record with this image, -- 2.40.1
signature.asc (application/pgp-signature, 832B) download ^ permalink raw reply [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 06:33     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 18:23       ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Andres Freund <andres@anarazel.de>
  2023-06-27 23:11         ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-28 22:57           ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-07-03 02:26             ` Michael Paquier <michael@paquier.xyz>
  0 siblings, 0 replies; 10+ messages in thread

From: Michael Paquier @ 2023-07-03 02:26 UTC (permalink / raw)
  To: Andres Freund <andres@anarazel.de>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
@ 2023-06-27 17:03     ` Nathan Bossart <nathandbossart@gmail.com>
  2023-06-27 23:19       ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  1 sibling, 1 reply; 10+ messages in thread

From: Nathan Bossart @ 2023-06-27 17:03 UTC (permalink / raw)
  To: Michael Paquier <michael@paquier.xyz>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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





^ permalink  raw  reply  [nested|flat] 10+ messages in thread

* Re: BUG #18000: Access method used by matview can be dropped leaving broken matview
  2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
  2023-06-26 08:35 ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 04:50   ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Michael Paquier <michael@paquier.xyz>
  2023-06-27 17:03     ` Re: BUG #18000: Access method used by matview can be dropped leaving broken matview Nathan Bossart <nathandbossart@gmail.com>
@ 2023-06-27 23:19       ` Michael Paquier <michael@paquier.xyz>
  0 siblings, 0 replies; 10+ messages in thread

From: Michael Paquier @ 2023-06-27 23:19 UTC (permalink / raw)
  To: Nathan Bossart <nathandbossart@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

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

Attachments:

  signature.asc (application/pgp-signature, 832B) download

^ permalink  raw  reply  [nested|flat] 10+ messages in thread


end of thread, other threads:[~2023-07-03 02:26 UTC | newest]

Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-26 08:00 BUG #18000: Access method used by matview can be dropped leaving broken matview PG Bug reporting form <noreply@postgresql.org>
2023-06-26 08:35 ` Michael Paquier <michael@paquier.xyz>
2023-06-27 04:50   ` Michael Paquier <michael@paquier.xyz>
2023-06-27 06:33     ` Michael Paquier <michael@paquier.xyz>
2023-06-27 18:23       ` Andres Freund <andres@anarazel.de>
2023-06-27 23:11         ` Michael Paquier <michael@paquier.xyz>
2023-06-28 22:57           ` Michael Paquier <michael@paquier.xyz>
2023-07-03 02:26             ` Michael Paquier <michael@paquier.xyz>
2023-06-27 17:03     ` Nathan Bossart <nathandbossart@gmail.com>
2023-06-27 23:19       ` Michael Paquier <michael@paquier.xyz>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox