Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 29 Jun 2023 07:57:40 +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: <ZJy65NH4MuOuuaBW@paquier.xyz>
References: <18000-9145c25b1af475ca@postgresql.org>
 <ZJlN4DFm4FvVxe6v@paquier.xyz>
 <ZJpqh3xoFtlwZS9u@paquier.xyz>
 <ZJqCusodr0jYZgMq@paquier.xyz>
 <20230627182332.uq6afduhvdip7pol@awork3.anarazel.de>
 <ZJtst5BUaRnH6JSj@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="ozkDcIuczTaFh9HR"
Content-Disposition: inline
In-Reply-To: <ZJtst5BUaRnH6JSj@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZJy65NH4MuOuuaBW%40paquier.xyz>
Precedence: bulk


--ozkDcIuczTaFh9HR
Content-Type: multipart/mixed; boundary="LGfDqY8F9smWDji1"
Content-Disposition: inline


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

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

--LGfDqY8F9smWDji1
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v3-0001-Fix-pg_depend-entry-to-AMs-after-ALTER-TABLE-.-SE.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 39ddc35db15222a7b5d6ee94fa27023e12112ae9 Mon Sep 17 00:00:00 2001
=46rom: 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 ACC=
ESS
 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;
=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,31 @@ 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 relations to point to their 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));
+		if (changeDependencyFor(RelationRelationId,
+								r2,
+								AccessMethodRelationId,
+								relam2,
+								relam1) !=3D 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_waldum=
p.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);
=20
-		snprintf(filename, MAXPGPATH, "%s/%08X-%08X.%u.%u.%u.%u%s", savepath,
+		snprintf(filename, MAXPGPATH, "%s/%08X-%08X-%08X.%u.%u.%u.%u%s", savepat=
h,
+				 record->seg.ws_tli,
 				 LSN_FORMAT_ARGS(record->ReadRecPtr),
 				 rnode.spcOid, rnode.dbOid, rnode.relNumber, blk, forkname);
=20
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');
=20
 # 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 =3D
-  qr/^([0-9A-F]{8})-([0-9A-F]{8})[.][0-9]+[.][0-9]+[.][0-9]+[.][0-9]+(?:_v=
m|_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)?$/;
=20
 my $file_count =3D 0;
=20
diff --git a/src/test/regress/expected/create_am.out b/src/test/regress/exp=
ected/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)
=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..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 =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;
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump=
=2Esgml
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</repl=
aceable>.<replaceable>BLKNO</replaceable><replaceable>FORK</replaceable></l=
iteral>
+        <literal><replaceable>TIMELINE</replaceable>-<replaceable>LSN</rep=
laceable>.<replaceable>RELTABLESPACE</replaceable>.<replaceable>DATOID</rep=
laceable>.<replaceable>RELNODE</replaceable>.<replaceable>BLKNO</replaceabl=
e><replaceable>FORK</replaceable></literal>
=20
         The file names are composed of the following parts:
         <informaltable>
@@ -296,6 +296,13 @@ PostgreSQL documentation
           </thead>
=20
           <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 imag=
e,
--=20
2.40.1


--LGfDqY8F9smWDji1--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmScuuQACgkQnvQgOdby
QH2Zwg/+P8yImxqGcQ+VF6mLl3rpXrCfZBnWvq1xSniU62a752V6Mn/PjCGR0Jrl
Swataw2G1jblc3zEUhbuy529FXudpO05ROUJPjSmXvKpKbYHpT6K5sriIDpoONsg
yYSwKCNKY2LkAtB5F7hJOGwROvypZp+VV8/OG+f5IAjzntYKjPtBiNmCpsGx/1NA
pZRz43Ov8cSv3fTnStaRk357EtfpnVGJu3cKrDjV2M6UZ/x03kTVu0upHp1mNoAY
farJK7M8rdfT4cm0DwaO03dhAYtNuAeKxQu+Lcb9kDeDTjFaXdYP+PnIvVOiKXe+
b5tnVTxo5logdvs5GOf6403H5aqk/xH9qJGaZceUtGlpGsakZMsxv6iSuzEKgdXO
6fRG2bLczw6B6PcM9G5hHwiyqH+ASiFPlIN8jagW9g73NbW0LiG6IYl2qTH0kEIF
PYNTZTAEKNHgDU9xlMEVCQgoeFwm6hJ5kdd1qr8N+rUHhHboZtvIC2hA/3Yay/Dt
TSTrXgPV5E/OWgi9rOTGS/01ZFQuHRmH3eqoS/mTOTMpYSvSBKGAjza2abriAELV
pPV28nYkxAkUYde6CFTvsBlOoNt+MLpf2bbMMfck0fb7rzouaAaRmvC5gUiWB6+e
iOE/HXS24AwjHtDg62rS+VzKWgmTqaIUPPCapXpnTFxe3VoDoso=
=xyXj
-----END PGP SIGNATURE-----

--ozkDcIuczTaFh9HR--