Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 28 Jun 2023 08:11:51 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Andres Freund <andres@anarazel.de>
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: <ZJtst5BUaRnH6JSj@paquier.xyz>
References: <18000-9145c25b1af475ca@postgresql.org>
 <ZJlN4DFm4FvVxe6v@paquier.xyz>
 <ZJpqh3xoFtlwZS9u@paquier.xyz>
 <ZJqCusodr0jYZgMq@paquier.xyz>
 <20230627182332.uq6afduhvdip7pol@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="3D3xTvw5EuFvTiVS"
Content-Disposition: inline
In-Reply-To: <20230627182332.uq6afduhvdip7pol@awork3.anarazel.de>
Archived-At: 
 <https://www.postgresql.org/message-id/ZJtst5BUaRnH6JSj%40paquier.xyz>
Precedence: bulk


--3D3xTvw5EuFvTiVS
Content-Type: multipart/mixed; boundary="PG4vCoX/zS2ujY8l"
Content-Disposition: inline


--PG4vCoX/zS2ujY8l
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--PG4vCoX/zS2ujY8l
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Fix-dependencies-under-ALTER-TABLE-SET-ACCESS-MET.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 6ef48042e1eefd30b16438c03e584b41733b3e49 Mon Sep 17 00:00:00 2001
=46rom: 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;
=20
 	/* We need writable copies of both pg_class tuples. */
 	relRelation =3D table_open(RelationRelationId, RowExclusiveLock);
@@ -1086,6 +1088,8 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_pg=
_class,
=20
 	relfilenumber1 =3D relform1->relfilenode;
 	relfilenumber2 =3D relform2->relfilenode;
+	relam1 =3D relform1->relam;
+	relam2 =3D relform2->relam;
=20
 	if (RelFileNumberIsValid(relfilenumber1) &&
 		RelFileNumberIsValid(relfilenumber2))
@@ -1257,6 +1261,23 @@ swap_relation_files(Oid r1, Oid r2, bool target_is_p=
g_class,
 		CacheInvalidateRelcacheByTuple(reltup2);
 	}
=20
+	/*
+	 * 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 !=3D relam2)
+	{
+		if (changeDependencyFor(RelationRelationId,
+								r1,
+								AccessMethodRelationId,
+								relam1,
+								relam2) !=3D 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/exp=
ected/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)
=20
+-- 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 =3D 'pg_class'::regclass AND
+        objid =3D 'heaptable'::regclass
+  ORDER BY 1, 2;
+       obj       |       objref        | deptype=20
+-----------------+---------------------+---------
+ 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 =3D 'pg_class'::regclass AND
+        objid =3D 'heaptable'::regclass
+  ORDER BY 1, 2;
+       obj       |    objref     | deptype=20
+-----------------+---------------+---------
+ 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 =3D am.oid AND c.oid =3D 'heaptable'::regclass;
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/crea=
te_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 =3D am.oid AND c.oid =3D '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 =3D 'pg_class'::regclass AND
+        objid =3D '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 =3D 'pg_class'::regclass AND
+        objid =3D '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 =3D am.oid AND c.oid =3D 'heaptable'::regclass;
--=20
2.40.1


--PG4vCoX/zS2ujY8l--

--3D3xTvw5EuFvTiVS
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmSbbLcACgkQnvQgOdby
QH2UFg//Tvm/yLx1rpOrrU684iVgcHeYvYas9/cF4pRPbgU5xgCoSb6u/rXzQU0Q
1znZio0gP7C7lKDHQTFrfI4D2/Zpm6OZqzrDRslOxZALxSRe2ho4CeZK7a/ath7C
ZL9mWDtJio0e+PIVksgqpF1EAnc0GuyYS80xXV8srMScd00SY6iCSqkRYDF8nQBZ
BybbyCTGw0NdiITis80/6dEm3kq3fKffcTv0HRTZYYFiClSD94G9LkE1qWl2lUMe
xj+SOktlGUDNxMiG4lNC5k8GWLQxxJTLVktYRznhHHwgkv4NDQJdkCEbrdGzeN4D
GeWnLRauxkOgB0vX6CqLT4C/5E5W3UodMNgHtLeQ1flaCMfA1zetwitsaLmvR3mG
2Eygusqe2fcKQ/Z/Nyy+kBWVWRz8SlVg3prBihjDcCP5vviGBwBCNyAFAmNToCuc
lQ1oDgyVOUylAgX0jSgPOJ511ssT7TlCe/2ODjwGYKtShhGH3RGntGuOBCYVZejb
4Xdo77xoKWMamkFODYBIYU++74CuzqDwoSPDRwPrYebZWulbtBJ8jgUIfUnfoTWj
6LBcPcPHRg2aPwxRmQ8pV3mV660EDJqBCPxH8NRTJx7g2NoT6ClMXGjtlZruYJRa
nHQTPfiMmO2E7tQd2ZCgHM0vTEGuTzQay11nvRbnJZ2NPFqCTW8=
=unqF
-----END PGP SIGNATURE-----

--3D3xTvw5EuFvTiVS--