From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Andreas Karlsson <andreas@proxel.se>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: reindex concurrently and two toast indexes
Date: Tue, 10 Mar 2020 12:09:42 +0900
Message-ID: <20200310030942.GD4369@paquier.xyz> (raw)
In-Reply-To: <20200309070427.qqy4qehu3kgipwqk@nol>
References: <20200303080642.GA1890@paquier.xyz>
<20200303092551.GB1890@paquier.xyz>
<20200304051510.GE2593@paquier.xyz>
<CAOBaU_Z=qkE42VQ25mps+zyAz+WUn3wZoev4uhGW+gAXBBS2YA@mail.gmail.com>
<20200305035354.GQ2593@paquier.xyz>
<20200305165707.GA35281@nol>
<20200306013844.GE52814@paquier.xyz>
<20200306123648.GA49431@nol>
<20200309055231.GC96055@paquier.xyz>
<20200309070427.qqy4qehu3kgipwqk@nol>
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
Attachments:
[text/x-diff] 0001-Add-isolation-test-to-check-dependency-handling-of-i.patch (4.5K, ../20200310030942.GD4369@paquier.xyz/2-0001-Add-isolation-test-to-check-dependency-handling-of-i.patch)
download | inline diff:
From fd988b4141725082b30148da31d9c2a6a88c7319 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@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.outnew file mode 100644index 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_scheduleindex 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.specnew file mode 100644index 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
[application/pgp-signature] signature.asc (832B, ../20200310030942.GD4369@paquier.xyz/3-signature.asc)
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-hackers@postgresql.org
Cc: michael@paquier.xyz, rjuju123@gmail.com, pryzby@telsasoft.com, sk@zsrv.org, peter.eisentraut@2ndquadrant.com, michael.paquier@gmail.com, andreas@proxel.se
Subject: Re: reindex concurrently and two toast indexes
In-Reply-To: <20200310030942.GD4369@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