From: Michael Paquier <michael@paquier.xyz>
To: Noah Misch <noah@leadboat.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Alexey Ermakov <alexey.ermakov@dataegret.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Maxim Boguk <maxim.boguk@gmail.com>
Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17268: Possible corruption in toast index after reindex index concurrently
Date: Mon, 15 Nov 2021 20:03:04 +0900
Message-ID: <YZI+aNEnnpBASxNU@paquier.xyz> (raw)
In-Reply-To: <YYzd3dsYi/LiwWuc@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>
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
Attachments:v1-0001-Fix-locking-of-toast-relations-with-REINDEX-CONC.patch (text/x-diff, 12.1K) download
From 0cdf4982f4a0a4211752fc0228b80b4d89ef07ad Mon Sep 17 00:00:00 2001
From: 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/access/common/toast_internals.cindex 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);
/*
* Create the TOAST pointer value that we'll return
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.cindex c14ca27c5e..8370d1ab6d 100644--- a/src/backend/commands/indexcmds.c+++ b/src/backend/commands/indexcmds.c@@ -3262,14 +3262,7 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
* toast indexes.
*/
Relation heapRelation;
-- /* Save the list of relation OIDs in private context */- oldcontext = MemoryContextSwitchTo(private_context);-- /* Track this relation for session locks */- heapRelationIds = lappend_oid(heapRelationIds, relationOid);-- MemoryContextSwitchTo(oldcontext);+ Oid relLockedOid = relationOid;
if (IsCatalogRelationOid(relationOid))
ereport(ERROR,
@@ -3296,6 +3289,46 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
errmsg("cannot move system relation \"%s\"",
RelationGetRelationName(heapRelation))));
+ /*+ * If querying a toast relation, locks are taken on its+ * parent table instead.+ */+ relLockedOid = relationOid;+ if (relkind == RELKIND_TOASTVALUE)+ {+ Relation classRel;+ TableScanDesc relScan;+ ScanKeyData key;+ HeapTuple tuple;+ Form_pg_class classForm;++ classRel = table_open(RelationRelationId, AccessShareLock);+ ScanKeyInit(&key,+ Anum_pg_class_reltoastrelid,+ BTEqualStrategyNumber, F_OIDEQ,+ ObjectIdGetDatum(relationOid));++ relScan = table_beginscan_catalog(classRel, 1, &key);+ tuple = heap_getnext(relScan, ForwardScanDirection);++ if (!HeapTupleIsValid(tuple))+ elog(ERROR, "could not find toast tuple for relation %u",+ relationOid);++ classForm = (Form_pg_class) GETSTRUCT(tuple);+ relLockedOid = classForm->oid;+ table_endscan(relScan);+ table_close(classRel, AccessShareLock);+ }++ /* Save the list of relation OIDs in private context */+ oldcontext = MemoryContextSwitchTo(private_context);++ /* Track this relation for session locks */+ heapRelationIds = 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, ReindexParams *params)
(params->options & REINDEXOPT_MISSING_OK) != 0);
Relation heapRelation;
ReindexIndexInfo *idx;
+ Oid relLockedOid;
/* if relation is missing, leave */
if (!OidIsValid(heapId))
@@ -3442,11 +3476,45 @@ ReindexRelationConcurrently(Oid relationOid, ReindexParams *params)
table_close(heapRelation, NoLock);
+ /*+ * 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 = heapId;+ if (IsToastNamespace(get_rel_namespace(relationOid)))+ {+ Relation classRel;+ TableScanDesc relScan;+ ScanKeyData key;+ HeapTuple tuple;+ Form_pg_class classForm;++ classRel = table_open(RelationRelationId, AccessShareLock);+ ScanKeyInit(&key,+ Anum_pg_class_reltoastrelid,+ BTEqualStrategyNumber, F_OIDEQ,+ ObjectIdGetDatum(heapId));++ relScan = table_beginscan_catalog(classRel, 1, &key);++ tuple = heap_getnext(relScan, ForwardScanDirection);+ if (!HeapTupleIsValid(tuple))+ elog(ERROR, "could not find toast tuple for relation %u",+ relationOid);++ classForm = (Form_pg_class) GETSTRUCT(tuple);+ relLockedOid = classForm->oid;+ table_endscan(relScan);+ table_close(classRel, AccessShareLock);+ }+
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
/* Track the heap relation of this index for session locks */
- heapRelationIds = list_make1_oid(heapId);+ heapRelationIds = list_make1_oid(relLockedOid);
/*
* Save the list of relation OIDs in private context. Note
diff --git a/src/test/isolation/expected/reindex-concurrently-toast.out b/src/test/isolation/expected/reindex-concurrently-toast.outnew file mode 100644index 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; <waiting ...>+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 = (SELECT repeat('4', 11) || string_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i)) WHERE id = 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 = (SELECT repeat('4', 11) || string_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i)) WHERE id = 1;+step reind2: REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; <waiting ...>+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 = 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 = 2;+step reind2: REINDEX INDEX CONCURRENTLY pg_toast.reind_con_toast_idx; <waiting ...>+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/isolation_scheduleindex 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.specnew file mode 100644index 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 performed+# concurrently on toast relations. Toast relation names are not deterministic,+# 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 CONCURRENTLY,+# which cannot be launched in a transaction context.++# Create a table, with deterministic names for its toast relation and indexes.+# 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 = '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 = (SELECT oid FROM pg_class where relname = 'reind_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 = (SELECT repeat('4', 11) || string_agg(g.i::text || random()::text, '') FROM generate_series(1, 500) g(i)) WHERE id = 1; }+step del1 { DELETE FROM reind_con_wide WHERE id = 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--
2.33.1
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, noah@leadboat.com, andres@anarazel.de, alexey.ermakov@dataegret.com, robertmhaas@gmail.com, maxim.boguk@gmail.com, pgsql-bugs@lists.postgresql.org, pg@bowt.ie
Subject: Re: BUG #17268: Possible corruption in toast index after reindex index concurrently
In-Reply-To: <YZI+aNEnnpBASxNU@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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