Date: Tue, 10 Mar 2020 12:09:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Sergei Kornilov <sk@zsrv.org>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	Michael Paquier <michael.paquier@gmail.com>,
	Andreas Karlsson <andreas@proxel.se>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: reindex concurrently and two toast indexes
Message-ID: <20200310030942.GD4369@paquier.xyz>
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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="84ND8YJRMFlzkrP4"
Content-Disposition: inline
In-Reply-To: <20200309070427.qqy4qehu3kgipwqk@nol>
Precedence: bulk


--84ND8YJRMFlzkrP4
Content-Type: multipart/mixed; boundary="NtwzykIc2mflq5ck"
Content-Disposition: inline


--NtwzykIc2mflq5ck
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 :/
>=20
> Yes, unfortunately I don't see an acceptable way to add tests for that wi=
thout
> 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):=20
+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

--NtwzykIc2mflq5ck
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Add-isolation-test-to-check-dependency-handling-of-i.patch"
Content-Transfer-Encoding: quoted-printable

=46rom fd988b4141725082b30148da31d9c2a6a88c7319 Mon Sep 17 00:00:00 2001
=46rom: 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/iso=
lation/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 s=
1_drop s2_check
+step s1_select: SELECT data FROM reind_con_invalid FOR UPDATE;
+data          =20
+
+foo           =20
+bar           =20
+step s2_timeout: SET statement_timeout =3D 1000;
+step s2_reindex: REINDEX TABLE CONCURRENTLY reind_con_invalid; <waiting ..=
=2E>
+step s2_reindex: <... completed>
+ERROR:  canceling statement due to statement timeout
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_to=
ast_)([0-9+/=3D]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid =3D c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid =3D t.oid
+					    WHERE c.relname =3D 'reind_con_invalid'
+					      ORDER BY c.relname;
+regexp_replace indisvalid    =20
+
+pg_toast.pg_toast_<oid>_index_ccoldf             =20
+pg_toast.pg_toast_<oid>_indext             =20
+step s1_commit: COMMIT;
+step s1_drop: DROP TABLE reind_con_invalid;
+step s2_check: SELECT regexp_replace(i.indexrelid::regclass::text, '(pg_to=
ast_)([0-9+/=3D]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid =3D c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid =3D t.oid
+					    WHERE c.relname =3D 'reind_con_invalid'
+					      ORDER BY c.relname;
+regexp_replace indisvalid    =20
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/iso=
lation_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/isola=
tion/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 =3D 1000; }
+step "s2_reindex" { REINDEX TABLE CONCURRENTLY reind_con_invalid; }
+step "s2_check" { SELECT regexp_replace(i.indexrelid::regclass::text, '(pg=
_toast_)([0-9+/=3D]+)(_index)', '\1<oid>\3'),
+					         i.indisvalid
+					    FROM pg_class c
+					      JOIN pg_class t ON t.oid =3D c.reltoastrelid
+					      JOIN pg_index i ON i.indrelid =3D t.oid
+					    WHERE c.relname =3D '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"
--=20
2.25.1


--NtwzykIc2mflq5ck--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl5nBPYACgkQnvQgOdby
QH2rVBAAnrttEN/Sp66E0JAzI7k0xrcb65NfSDL5tTLoKGOvTptHwgnWl6+QnZiV
xw9w1uT0zfWjY2liX1juf/oulNE1oUR/H0c+aRZg4JpFR1c+NNyLZQ3VKPGm97qn
vRTNBedSw4rm3eRJ60gZQghkk7RLmhnm75i6QQr4gzII7FZW9IuvijuxXqCvQ8Wz
PsU4TQa6pXXdivozh05LSHsI+lxiwS5dAsKemPiAq2JngEEu8UOY2vUTV/J16pK+
+RkOUt3r1CJTGZlF1viF+UJfeg/773vcDN3Kz3JAdr7nq/aP62MitYTaCPHdzaZX
sibz2jHkqCesBZsOIPpPjSnPRR5QB/7WPZXtMFQyu7egcX8Th2sDQGZnCuQ34NGX
cI7YXyCJLcdZr2culCubrLxhB3/eBT3viF2c1vBv53ZuV2WeYb5O2L5ZEpAcrUvv
JszXJPUkHzK7dHGf8nFCVa9cz6mwM05jhY+sqsQ9MFhoB14RW5Cwm3WUibFYNTR1
n0CY3VLUbN3Kbh5/ApEJ3aTkYYxsg1jg1gCNiKP/RcMzmun20K+D1ovaX3m/syNY
S5jfUd355h1Xfd/EuAXjgduSSOX7Zf2IvPjYxv5socbIMqztWhKecY6MHSxpB7jl
MKQsW1D9+PDB4JmcqGU7sZMZtSe/kUeIdoyWg7PbF7owHIaVv0s=
=a16C
-----END PGP SIGNATURE-----

--84ND8YJRMFlzkrP4--