Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 28 Oct 2024 16:23:33 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18674: Partitioned table doesn't depend on access method it
 uses
Message-ID: <Zx879T_oFkiHv9St@paquier.xyz>
References: <18674-1ef01eceec278fab@postgresql.org>
 <CALdSSPgvKRX1pZE7GLKpDESw0wbBH4qBZPQSdSKYY2btCrfuSQ@mail.gmail.com>
 <CALdSSPjGzuo0eodxdP9WZS4Q7nvv-2k3W=9ie2XoMedBjkr1Fw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="lEEA2g6FzbvYs70f"
Content-Disposition: inline
In-Reply-To: 
 <CALdSSPjGzuo0eodxdP9WZS4Q7nvv-2k3W=9ie2XoMedBjkr1Fw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Zx879T_oFkiHv9St%40paquier.xyz>
Precedence: bulk


--lEEA2g6FzbvYs70f
Content-Type: multipart/mixed; boundary="YmrJRlMBofq2evL0"
Content-Disposition: inline


--YmrJRlMBofq2evL0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Oct 28, 2024 at 01:23:23AM +0500, Kirill Reshke wrote:
> Seems that simply changing RELKIND_HAS_TABLE_AM marco is too invasive.
> However, I will make another try. In v2 version, regression tests
> still fail, i will adjust them, if patching this way is the right
> approach to fix the problem..
>=20
> script provided by Alexander:
> CREATE ACCESS METHOD ham TYPE TABLE HANDLER heap_tableam_handler;
> CREATE TABLE pt (a int) PARTITION BY LIST (a) USING ham;
> DROP ACCESS METHOD ham;

 #define RELKIND_HAS_TABLE_AM(relkind) \
 	((relkind) =3D=3D RELKIND_RELATION || \
+	 (relkind) =3D=3D RELKIND_PARTITIONED_TABLE || \
 	 (relkind) =3D=3D RELKIND_TOASTVALUE || \
 	 (relkind) =3D=3D RELKIND_MATVIEW)

This is an incorrect approach to the problem.  RELKIND_HAS_TABLE_AM()
should be true only for relkinds that have rd_tableam set in their
relcache entries.  However, you are breaking the original promise of
this macro because it is not set (and should not be set) for
partitioned tables as these have no physical presence on disk.
pg_class.relam is just a reference to use when creating partitions on
it.  Note the comment in pg_class.h explaining the case of partitioned
tables.

Using an ALTER TABLE .. SET ACCESS METHOD on the partitioned table and
attempting a DROP ACCESS METHOD would fail.  We are missing a normal
dependency entry in pg_depend for your sequence of commands between
the new partitioned table with the USING clause and its access method
set.

The path of adding the dependency between an AM and a table/matview is
heap_create_with_catalog() when these are defined.  We can just expand
that for partitioned tables and simply fix the problem.  Attached is a
patch to do that, with tests around create_am.sql based on USING
checking the contents of pg_depend.
--
Michael

--YmrJRlMBofq2evL0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Fix-dependency-of-partitioned-table-and-its-table-AM.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 3bdc73cb92633cf1651d4c7e30691ac13809ef91 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 28 Oct 2024 16:11:49 +0900
Subject: [PATCH] Fix dependency of partitioned table and its table AM under
 USING

---
 src/backend/catalog/heap.c              |  3 ++-
 src/test/regress/expected/create_am.out | 16 ++++++++++++++++
 src/test/regress/sql/create_am.sql      | 12 ++++++++++++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 0078a12f26..c815019241 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1473,7 +1473,8 @@ heap_create_with_catalog(const char *relname,
 		 * No need to add an explicit dependency for the toast table, as the
 		 * main table depends on it.
 		 */
-		if (RELKIND_HAS_TABLE_AM(relkind) && relkind !=3D RELKIND_TOASTVALUE)
+		if ((RELKIND_HAS_TABLE_AM(relkind) && relkind !=3D RELKIND_TOASTVALUE) ||
+			relkind =3D=3D RELKIND_PARTITIONED_TABLE)
 		{
 			ObjectAddressSet(referenced, AccessMethodRelationId, accessmtd);
 			add_exact_object_address(&referenced, addrs);
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/exp=
ected/create_am.out
index 35d4cf1d46..08a3eec142 100644
--- a/src/test/regress/expected/create_am.out
+++ b/src/test/regress/expected/create_am.out
@@ -343,6 +343,22 @@ ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap,=
 SET ACCESS METHOD heap2;
 ERROR:  cannot have multiple SET ACCESS METHOD subcommands
 DROP MATERIALIZED VIEW heapmv;
 DROP TABLE heaptable;
+-- Partitioned table with USING
+BEGIN;
+CREATE TABLE am_partitioned(x INT, y INT) PARTITION BY hash (x) USING heap=
2;
+SELECT pg_describe_object(classid, objid, objsubid) AS obj,
+       pg_describe_object(refclassid, refobjid, refobjsubid) as refobj
+  FROM pg_depend, pg_am
+  WHERE pg_depend.refclassid =3D 'pg_am'::regclass
+    AND pg_am.oid =3D pg_depend.refobjid
+    AND pg_depend.objid =3D 'am_partitioned'::regclass;
+         obj          |       refobj       =20
+----------------------+---------------------
+ table am_partitioned | access method heap2
+(1 row)
+
+DROP TABLE am_partitioned;
+COMMIT;
 -- Partition hierarchies with access methods
 BEGIN;
 SET LOCAL default_table_access_method =3D 'heap';
diff --git a/src/test/regress/sql/create_am.sql b/src/test/regress/sql/crea=
te_am.sql
index 825aed325e..c43637227e 100644
--- a/src/test/regress/sql/create_am.sql
+++ b/src/test/regress/sql/create_am.sql
@@ -217,6 +217,18 @@ ALTER MATERIALIZED VIEW heapmv SET ACCESS METHOD heap,=
 SET ACCESS METHOD heap2;
 DROP MATERIALIZED VIEW heapmv;
 DROP TABLE heaptable;
=20
+-- Partitioned table with USING
+BEGIN;
+CREATE TABLE am_partitioned(x INT, y INT) PARTITION BY hash (x) USING heap=
2;
+SELECT pg_describe_object(classid, objid, objsubid) AS obj,
+       pg_describe_object(refclassid, refobjid, refobjsubid) as refobj
+  FROM pg_depend, pg_am
+  WHERE pg_depend.refclassid =3D 'pg_am'::regclass
+    AND pg_am.oid =3D pg_depend.refobjid
+    AND pg_depend.objid =3D 'am_partitioned'::regclass;
+DROP TABLE am_partitioned;
+COMMIT;
+
 -- Partition hierarchies with access methods
 BEGIN;
 SET LOCAL default_table_access_method =3D 'heap';
--=20
2.45.2


--YmrJRlMBofq2evL0--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmcfO/UACgkQnvQgOdby
QH16qw/+Kz+ResdiwmvAiKAUcte/qaPuX06YlKGcjPAsInpunZ7nre98UTOrsMvT
+Udswl+xPidbsdU4QvpKyWEfqi+mLwqIOXvCkskz/RlJMpmZc2f+4Sc36UNC0Z1J
woRr9DRoFE5HdnO0mb2j20hW5CvPRdiXSjlUs8D6/DUoJ3IIG6A74k4OWAsWNq1i
uFYEeyT5m5N5EZGnQAjzZrSIJOLOX9zq480XAeoQPnQk4vH6Sj3KBCVGe1gAAlm9
YE+07IZVSt//RL1Pe0bCVWrXmBNodRD+0sAywKGegVtTrcO1pHUkr/U8qIU583Nt
0flI6ko5o2AppxNLPSmTE1xJL01wCZ+bz5b/sg3D8lpBjo/0BL+8aJUXKG6CWgn3
PwlkjhdSWw2wvd498VZ+8OHwiIzSDtUEvUZ8wp0C9IS8T/nD1AtNekTCpmVxQUY9
5uhfqC0mR9KTeaOdvIUF9Kl6386+9+MltciKyZ52xuSGw8D9MQRmmxD7rd4MBp5L
/9J72pj7doEatc9rKKeujt9ur3PtuRcueYSy5h5G8G6eaqa7fPtOtsoh3MW6VcAB
kKMtVA7PvGMH8KLoMyEkZnqrc45MnEN4lT+xcHBmSgntnikmMN8LBhPK+DzTe7oy
Goum13N+rna4MZ93QoDrMF61mxDIbpU4oBK7Ju9ix0G2nYOveGc=
=gRk3
-----END PGP SIGNATURE-----

--lEEA2g6FzbvYs70f--