On Mon, Mar 09, 2020 at 08:04:27AM +0100, Julien Rouhaud wrote: > On Mon, Mar 09, 2020 at 02:52:31PM +0900, Michael Paquier wrote: >> For the index-level operation, issuing a WARNING is not consistent >> with the existing practice to use an ERROR, which is more adapted as >> the operation is done on a single index at a time. > > Agreed.
Thanks for checking the patch. >> It would have been nice to test this stuff. However, this requires an >> invalid toast index and we cannot create that except by canceling a >> concurrent reindex, leading us back to the upthread discussion about >> isolation tests, timeouts and fault injection :/ > > Yes, unfortunately I don't see an acceptable way to add tests for that without > some kind of fault injection, so this will have to wait :( Let's discuss that separately. I have also been reviewing the isolation test you have added upthread about the dependency handling of invalid indexes, and one thing that we cannot really do is attempting to do a reindex at index or table-level with invalid toast indexes as this leads to unstable ERROR or WARNING messages. But at least one thing we can do is to extend the query you sent directly so as it exposes the toast relation name filtered with regex_replace(). This gives us a stable output, and this way the test makes sure that the query cancellation happened after the dependencies are swapped, and not at build or validation time (indisvalid got appended to the end of the output): +pg_toast.pg_toast_<oid>_index_ccoldf +pg_toast.pg_toast_<oid>_indext Please feel free to see the attached for reference, that's not something for commit in upstream, but I am going to keep that around in my own plugin tree. -- Michael
From fd988b4141725082b30148da31d9c2a6a88c7319 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 10 Mar 2020 12:01:23 +0900
Subject: [PATCH] Add isolation test to check dependency handling of invalid
indexes
This uses a statement_timeout of 1s, which is not really a good idea :D
---
.../isolation/expected/reindex-invalid.out | 34 +++++++++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/reindex-invalid.spec | 38 +++++++++++++++++++
3 files changed, 73 insertions(+)
create mode 100644 src/test/isolation/expected/reindex-invalid.out
create mode 100644 src/test/isolation/specs/reindex-invalid.spec
diff --git a/src/test/isolation/expected/reindex-invalid.out b/src/test/isolation/expected/reindex-invalid.out
new file mode 100644
index 0000000000..ef1eff9ce7
--- /dev/null
+++ b/src/test/isolation/expected/reindex-invalid.out
@@ -0,0 +1,34 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_select s2_timeout s2_reindex s2_check s1_commit s1_drop s2_check
+step s1_select: SELECT data FROM reind_con_invalid FOR UPDATE;
+data
+
+foo
+bar
+step s2_timeout: SET statement_timeout = 1000;
+step s2_reindex: REINDEX TABLE CONCURRENTLY reind_con_invalid; <waiting ...>
+step s2_reindex: <... completed>
+ERROR: canceling statement due to statement timeout
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+ i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_invalid'
+ ORDER BY c.relname;
+regexp_replace indisvalid
+
+pg_toast.pg_toast_<oid>_index_ccoldf
+pg_toast.pg_toast_<oid>_indext
+step s1_commit: COMMIT;
+step s1_drop: DROP TABLE reind_con_invalid;
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+ i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_invalid'
+ ORDER BY c.relname;
+regexp_replace indisvalid
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 2873cd7c21..28e5b8b16a 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -48,6 +48,7 @@ test: lock-committed-update
test: lock-committed-keyupdate
test: update-locked-tuple
test: reindex-concurrently
+test: reindex-invalid
test: propagate-lock-delete
test: tuplelock-conflict
test: tuplelock-update
diff --git a/src/test/isolation/specs/reindex-invalid.spec b/src/test/isolation/specs/reindex-invalid.spec
new file mode 100644
index 0000000000..bf866d2279
--- /dev/null
+++ b/src/test/isolation/specs/reindex-invalid.spec
@@ -0,0 +1,38 @@
+# REINDEX CONCURRENTLY with invalid indexes
+#
+# Check that handling of dependencies with invalid indexes is correct.
+# When the parent table is dropped, all the indexes are dropped.
+
+setup
+{
+ CREATE TABLE reind_con_invalid (id serial primary key, data text);
+ INSERT INTO reind_con_invalid (data) VALUES ('foo');
+ INSERT INTO reind_con_invalid (data) VALUES ('bar');
+}
+
+# No need to drop the table at teardown phase here as each permutation
+# takes care of it internally.
+
+session "s1"
+setup { BEGIN; }
+step "s1_select" { SELECT data FROM reind_con_invalid FOR UPDATE; }
+step "s1_commit" { COMMIT; }
+step "s1_drop" { DROP TABLE reind_con_invalid; }
+
+session "s2"
+step "s2_timeout" { SET statement_timeout = 1000; }
+step "s2_reindex" { REINDEX TABLE CONCURRENTLY reind_con_invalid; }
+step "s2_check" { SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_toast_)([0-9+/=]+)(_index)', '\1<oid>\3'),
+ i.indisvalid
+ FROM pg_class c
+ JOIN pg_class t ON t.oid = c.reltoastrelid
+ JOIN pg_index i ON i.indrelid = t.oid
+ WHERE c.relname = 'reind_con_invalid'
+ ORDER BY c.relname; }
+
+# A failed REINDEX CONCURRENTLY will leave an invalid index on the toast
+# table of TOAST table. Any following successful REINDEX should leave
+# this index as invalid, otherwise we would end up with a useless and
+# duplicated index that can't be dropped. Any invalid index should be
+# dropped once the parent table is dropped.
+permutation "s1_select" "s2_timeout" "s2_reindex" "s2_check" "s1_commit" "s1_drop" "s2_check"
--
2.25.1
signature.asc
Description: PGP signature
