Date: Mon, 15 Nov 2021 20:03:04 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: Andres Freund <andres@anarazel.de>,
	Alexey Ermakov <alexey.ermakov@dataegret.com>,
	Robert Haas <robertmhaas@gmail.com>,
	Maxim Boguk <maxim.boguk@gmail.com>,
	PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>,
	Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17268: Possible corruption in toast index after reindex
 index concurrently
Message-ID: <YZI+aNEnnpBASxNU@paquier.xyz>
References: <20211108185317.fz42txmnkqdkpix6@alap3.anarazel.de>
 <20211108203641.whpia3pzjofellf4@alap3.anarazel.de>
 <20211109035624.GA940092@rfd.leadboat.com>
 <B706E617-2172-4460-845A-B76E5DB2E0A2@anarazel.de>
 <YYoA24hSkx6EYWpx@paquier.xyz>
 <20211109050848.oe3u6mhtomkaoxrx@alap3.anarazel.de>
 <20211109054625.GB940092@rfd.leadboat.com>
 <20211109063119.ukcvdk7uvz7ks3pa@alap3.anarazel.de>
 <20211109064217.GC940092@rfd.leadboat.com>
 <YYzd3dsYi/LiwWuc@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="r5NRi/93gsYxQm5f"
Content-Disposition: inline
In-Reply-To: <YYzd3dsYi/LiwWuc@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/YZI%2BaNEnnpBASxNU%40paquier.xyz>
Precedence: bulk


--r5NRi/93gsYxQm5f
Content-Type: multipart/mixed; boundary="ijqBg7BTjgZn1Wsa"
Content-Disposition: inline


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

On Thu, Nov 11, 2021 at 06:09:49PM +0900, Michael Paquier wrote:
> To be clear on this point, users cannot reindex concurrently catalog
> indexes and toast indexes associated to catalog tables, just toast
> indexes of normal tables.  I don't know if any of you have been
> working on a patch, but I was cooking something.  It would be worth
> checking if an isolation test could be written.

So, I have worked on this one.  And attached is a patch that
implements the two approaches suggested by Andres which are able to
fix the issues discussed:
1) Switch to a session-level lock on the parent relation if doing a
reindex concurrently on a toast table or on one of its indexes.  This
requires to look back at pg_class.reltoastrelid to find the correct
parent.  This stresses me quite a bit, and I am not sure that I like
that to be honest because we don't do anything like that in the rest
of the tree.  I am also getting the feeling that this is an open door
for more issues.
2) Don't release locks when a new toast value is saved until the end
of its transaction.

After more testing, I have been able to extract and write an isolation
test that is able to reproduce the failure.  It relies on a trick as
the toast relation names are not deterministic, and we cannot use
REINDEX CONCURRENTLY in a function context.  So I have used an ALTER
TABLE/INDEX RENAME with a DO block to change the toast relation
names with allow_system_table_mods instead.  There is no need either
for amcheck with this method.

2) is enough to fix the problem, and I'd like to think that we had
better stick with only this method for simplicity's sake.

Comments?
--
Michael

--ijqBg7BTjgZn1Wsa
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v1-0001-Fix-locking-of-toast-relations-with-REINDEX-CONC.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 0cdf4982f4a0a4211752fc0228b80b4d89ef07ad Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 15 Nov 2021 19:55:10 +0900
Subject: [PATCH v1] Fix locking of toast relations with REINDEX CONC

---
 src/backend/access/common/toast_internals.c   |  4 +-
 src/backend/commands/indexcmds.c              | 86 +++++++++++++++++--
 .../expected/reindex-concurrently-toast.out   | 85 ++++++++++++++++++
 src/test/isolation/isolation_schedule         |  1 +
 .../specs/reindex-concurrently-toast.spec     | 55 ++++++++++++
 5 files changed, 220 insertions(+), 11 deletions(-)
 create mode 100644 src/test/isolation/expected/reindex-concurrently-toast.=
out
 create mode 100644 src/test/isolation/specs/reindex-concurrently-toast.spec

diff --git a/src/backend/access/common/toast_internals.c b/src/backend/acce=
ss/common/toast_internals.c
index c7b9ade574..0a946631bb 100644
--- a/src/backend/access/common/toast_internals.c
+++ b/src/backend/access/common/toast_internals.c
@@ -361,8 +361,8 @@ toast_save_datum(Relation rel, Datum value,
 	/*
 	 * Done - close toast relation and its indexes
 	 */
-	toast_close_indexes(toastidxs, num_indexes, RowExclusiveLock);
-	table_close(toastrel, RowExclusiveLock);
+	toast_close_indexes(toastidxs, num_indexes, NoLock);
+	table_close(toastrel, NoLock);
=20
 	/*
 	 * Create the TOAST pointer value that we'll return
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexc=
mds.c
index c14ca27c5e..8370d1ab6d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3262,14 +3262,7 @@ ReindexRelationConcurrently(Oid relationOid, Reindex=
Params *params)
 				 * toast indexes.
 				 */
 				Relation	heapRelation;
-
-				/* Save the list of relation OIDs in private context */
-				oldcontext =3D MemoryContextSwitchTo(private_context);
-
-				/* Track this relation for session locks */
-				heapRelationIds =3D lappend_oid(heapRelationIds, relationOid);
-
-				MemoryContextSwitchTo(oldcontext);
+				Oid			relLockedOid =3D relationOid;
=20
 				if (IsCatalogRelationOid(relationOid))
 					ereport(ERROR,
@@ -3296,6 +3289,46 @@ ReindexRelationConcurrently(Oid relationOid, Reindex=
Params *params)
 							 errmsg("cannot move system relation \"%s\"",
 									RelationGetRelationName(heapRelation))));
=20
+				/*
+				 * If querying a toast relation, locks are taken on its
+				 * parent table instead.
+				 */
+				relLockedOid =3D relationOid;
+				if (relkind =3D=3D RELKIND_TOASTVALUE)
+				{
+					Relation classRel;
+					TableScanDesc relScan;
+					ScanKeyData key;
+					HeapTuple   tuple;
+					Form_pg_class classForm;
+
+					classRel =3D table_open(RelationRelationId, AccessShareLock);
+					ScanKeyInit(&key,
+								Anum_pg_class_reltoastrelid,
+								BTEqualStrategyNumber, F_OIDEQ,
+								ObjectIdGetDatum(relationOid));
+
+					relScan =3D table_beginscan_catalog(classRel, 1, &key);
+					tuple =3D heap_getnext(relScan, ForwardScanDirection);
+
+					if (!HeapTupleIsValid(tuple))
+						elog(ERROR, "could not find toast tuple for relation %u",
+							relationOid);
+
+					classForm =3D (Form_pg_class) GETSTRUCT(tuple);
+					relLockedOid =3D classForm->oid;
+					table_endscan(relScan);
+					table_close(classRel, AccessShareLock);
+				}
+
+				/* Save the list of relation OIDs in private context */
+				oldcontext =3D MemoryContextSwitchTo(private_context);
+
+				/* Track this relation for session locks */
+				heapRelationIds =3D lappend_oid(heapRelationIds, relLockedOid);
+
+				MemoryContextSwitchTo(oldcontext);
+
 				/* Add all the valid indexes of relation to list */
 				foreach(lc, RelationGetIndexList(heapRelation))
 				{
@@ -3394,6 +3427,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexP=
arams *params)
 													  (params->options & REINDEXOPT_MISSING_OK) !=3D 0);
 				Relation	heapRelation;
 				ReindexIndexInfo *idx;
+				Oid			relLockedOid;
=20
 				/* if relation is missing, leave */
 				if (!OidIsValid(heapId))
@@ -3442,11 +3476,45 @@ ReindexRelationConcurrently(Oid relationOid, Reinde=
xParams *params)
=20
 				table_close(heapRelation, NoLock);
=20
+				/*
+				 * If this is a toast index, save the parent table instead
+				 * of the toast relation itself for the session locks.  This
+				 * looks backwards at pg_class.reltoastrelid to know which
+				 * table this toast relation links to.
+				 */
+				relLockedOid =3D heapId;
+				if (IsToastNamespace(get_rel_namespace(relationOid)))
+				{
+					Relation classRel;
+					TableScanDesc relScan;
+					ScanKeyData key;
+					HeapTuple   tuple;
+					Form_pg_class classForm;
+
+					classRel =3D table_open(RelationRelationId, AccessShareLock);
+					ScanKeyInit(&key,
+								Anum_pg_class_reltoastrelid,
+								BTEqualStrategyNumber, F_OIDEQ,
+								ObjectIdGetDatum(heapId));
+
+					relScan =3D table_beginscan_catalog(classRel, 1, &key);
+
+					tuple =3D heap_getnext(relScan, ForwardScanDirection);
+					if (!HeapTupleIsValid(tuple))
+						elog(ERROR, "could not find toast tuple for relation %u",
+							relationOid);
+
+					classForm =3D (Form_pg_class) GETSTRUCT(tuple);
+					relLockedOid =3D classForm->oid;
+					table_endscan(relScan);
+					table_close(classRel, AccessShareLock);
+				}
+
 				/* Save the list of relation OIDs in private context */
 				oldcontext =3D MemoryContextSwitchTo(private_context);
=20
 				/* Track the heap relation of this index for session locks */
-				heapRelationIds =3D list_make1_oid(heapId);
+				heapRelationIds =3D list_make1_oid(relLockedOid);
=20
 				/*
 				 * Save the list of relation OIDs in private context.  Note
diff --git a/src/test/isolation/expected/reindex-concurrently-toast.out b/s=
rc/test/isolation/expected/reindex-concurrently-toast.out
new file mode 100644
index 0000000000..3b9023728a
--- /dev/null
+++ b/src/test/isolation/expected/reindex-concurrently-toast.out
@@ -0,0 +1,85 @@
+Parsed test spec with 2 sessions
+
+starting permutation: lock1 ins1 retab2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step ins1: INSERT INTO reind_con_wide SELECT 3, repeat('3', 11) || string_=
agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i);
+step retab2: REINDEX TABLE CONCURRENTLY pg_toast.reind_con_toast; <waiting=
 ...>
+step end1: COMMIT;
+step retab2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|1111111111
+ 2|2222222222
+ 3|3333333333
+(3 rows)
+
+
+starting permutation: lock1 ins1 reind2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step ins1: INSERT INTO reind_con_wide SELECT 3, repeat('3', 11) || string_=
agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i);
+step reind2: REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; <wai=
ting ...>
+step end1: COMMIT;
+step reind2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|1111111111
+ 2|2222222222
+ 3|3333333333
+(3 rows)
+
+
+starting permutation: lock1 upd1 retab2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step upd1: UPDATE reind_con_wide SET data =3D (SELECT repeat('4', 11) || s=
tring_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i=
)) WHERE id =3D 1;
+step retab2: REINDEX TABLE CONCURRENTLY pg_toast.reind_con_toast; <waiting=
 ...>
+step end1: COMMIT;
+step retab2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|4444444444
+ 2|2222222222
+(2 rows)
+
+
+starting permutation: lock1 upd1 reind2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step upd1: UPDATE reind_con_wide SET data =3D (SELECT repeat('4', 11) || s=
tring_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i=
)) WHERE id =3D 1;
+step reind2: REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; <wai=
ting ...>
+step end1: COMMIT;
+step reind2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|4444444444
+ 2|2222222222
+(2 rows)
+
+
+starting permutation: lock1 del1 retab2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step del1: DELETE FROM reind_con_wide WHERE id =3D 2;
+step retab2: REINDEX TABLE CONCURRENTLY pg_toast.reind_con_toast; <waiting=
 ...>
+step end1: COMMIT;
+step retab2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|1111111111
+(1 row)
+
+
+starting permutation: lock1 del1 reind2 end1 sel2
+step lock1: lock TABLE reind_con_wide in ROW EXCLUSIVE MODE;
+step del1: DELETE FROM reind_con_wide WHERE id =3D 2;
+step reind2: REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; <wai=
ting ...>
+step end1: COMMIT;
+step reind2: <... completed>
+step sel2: SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id;
+id|    substr
+--+----------
+ 1|1111111111
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/iso=
lation_schedule
index f4c01006fc..99c23b16ff 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -53,6 +53,7 @@ test: lock-committed-update
 test: lock-committed-keyupdate
 test: update-locked-tuple
 test: reindex-concurrently
+test: reindex-concurrently-toast
 test: reindex-schema
 test: propagate-lock-delete
 test: tuplelock-conflict
diff --git a/src/test/isolation/specs/reindex-concurrently-toast.spec b/src=
/test/isolation/specs/reindex-concurrently-toast.spec
new file mode 100644
index 0000000000..616396a8f1
--- /dev/null
+++ b/src/test/isolation/specs/reindex-concurrently-toast.spec
@@ -0,0 +1,55 @@
+# REINDEX CONCURRENTLY with toast relations
+#
+# Ensure that concurrent operations work correctly when a REINDEX is perfo=
rmed
+# concurrently on toast relations.  Toast relation names are not determini=
stic,
+# so this abuses of allow_system_table_mods to change the names of toast
+# tables and its indexes so as they can be executed with REINDEX CONCURREN=
TLY,
+# which cannot be launched in a transaction context.
+
+# Create a table, with deterministic names for its toast relation and inde=
xes.
+# Fortunately ALTER TABLE is transactional, making the renaming of toast
+# relations possible with allow_system_table_mods.
+setup
+{
+    CREATE TABLE reind_con_wide(id int primary key, data text);
+    INSERT INTO reind_con_wide
+      SELECT 1, repeat('1', 11) || string_agg(g.i::text || random()::text,=
 '') FROM generate_series(1, 500) g(i);
+    INSERT INTO reind_con_wide
+      SELECT 2, repeat('2', 11) || string_agg(g.i::text || random()::text,=
 '') FROM generate_series(1, 500) g(i);
+    SET allow_system_table_mods TO true;
+    DO $$DECLARE r record;
+    BEGIN
+    SELECT INTO r reltoastrelid::regclass::text AS table_name FROM pg_class
+      WHERE oid =3D 'reind_con_wide'::regclass;
+    EXECUTE 'ALTER TABLE ' || r.table_name || ' RENAME TO reind_con_toast;=
';
+    SELECT INTO r indexrelid::regclass::text AS index_name FROM pg_index
+        WHERE indrelid =3D (SELECT oid FROM pg_class where relname =3D 're=
ind_con_toast');
+    EXECUTE 'ALTER INDEX ' || r.index_name || ' RENAME TO reind_con_toast_=
idx;';
+    END$$;
+}
+
+teardown
+{
+    DROP TABLE reind_con_wide;
+}
+
+session s1
+setup { BEGIN; }
+step lock1 { lock TABLE reind_con_wide in ROW EXCLUSIVE MODE; }
+step ins1 { INSERT INTO reind_con_wide SELECT 3, repeat('3', 11) || string=
_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i); }
+step upd1 { UPDATE reind_con_wide SET data =3D (SELECT repeat('4', 11) || =
string_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(=
i)) WHERE id =3D 1; }
+step del1 { DELETE FROM reind_con_wide WHERE id =3D 2; }
+step end1 { COMMIT; }
+
+session s2
+step retab2 { REINDEX TABLE CONCURRENTLY pg_toast.reind_con_toast; }
+step reind2 { REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; }
+step sel2 { SELECT id, substr(data, 1, 10) FROM reind_con_wide ORDER BY id=
; }
+
+# Test REINDEX CONCURRENTLY on toast table and its index.
+permutation lock1 ins1 retab2 end1 sel2
+permutation lock1 ins1 reind2 end1 sel2
+permutation lock1 upd1 retab2 end1 sel2
+permutation lock1 upd1 reind2 end1 sel2
+permutation lock1 del1 retab2 end1 sel2
+permutation lock1 del1 reind2 end1 sel2
--=20
2.33.1


--ijqBg7BTjgZn1Wsa--

--r5NRi/93gsYxQm5f
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmGSPmgACgkQnvQgOdby
QH1FyBAAqDL3Ovqm1dC/SjVTNLlUU8hyJYs6iN5WoHM9t7G4aYG6kOXsta33K13N
BhYrRoZpQHJldhlaJzOMl0KXtPFB12LEETGS3BgeeAJuc6YUnziLv7w16x9zf281
lCcrEKp49qyNmtbkkE2TpEbWtRERP4H+GhSDCcVOua3wk/OPUIdtwALQ2x5XxoIf
NDMn/7zG/idBDQEdSn3jUWQBTZo3kZlJR/WfJva98sYph5Y59Vg2q6cV6X+6639I
qhjmW4/ZP+BnKx78yZos1f/415Bp7G3fRTsLdlXT/U53kZK1V/hnRTsHwKfrPAYI
PuWMsWgMPScNu3f33EV8c1Uw5rHWYfgzro2fxTi6JifaA1WnQBLOkYUJKkyH0+FO
EO7kjV/4PtDYNiIsDPJQafL15TPnxewgDNA0IbaCPWIS3ik0kEe5/ce4LRO3ZPsD
KD9+KS/90KXQDpjHGWdkvmbJbqepVCbEbExvHfOOFDWZp6QjcM/hWDOW+eUMNwGW
Of8dMxpeKAc/j5xmfwuRcph0NVYluc0bPUuOFAfGjhg5zAIbycTb2pYl8pjvy5Ei
Km8DV0PEFZDCbQaSAtN49fCRrqoTWemqlcK4TfiAnVpoF1itMga6yB4dQ19EA8XZ
bzkhkyDY+xYiElWMzq+tmSlgaA2m5/j+sajtmdEh1jOFsrUE2Fw=
=ICnF
-----END PGP SIGNATURE-----

--r5NRi/93gsYxQm5f--