pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedreindex concurrently and two toast indexes
22+ messages / 3 participants
[nested] [flat]
* reindex concurrently and two toast indexes
@ 2020-02-16 19:08 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 22+ messages in thread
From: Justin Pryzby @ 2020-02-16 19:08 UTC (permalink / raw)
To: Sergei Kornilov <sk@zsrv.org>; +Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andreas Karlsson <andreas@proxel.se>; pgsql-hackers
Forking old, long thread:
https://www.postgresql.org/message-id/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
On Fri, Jan 04, 2019 at 03:18:06PM +0300, Sergei Kornilov wrote:
> About reindex invalid indexes - i found one good question in archives [1]: how about toast indexes?
> I check it now, i am able drop invalid toast index, but i can not drop reduntant valid index.
> Reproduce:
> session 1: begin; select from test_toast ... for update;
> session 2: reindex table CONCURRENTLY test_toast ;
> session 2: interrupt by ctrl+C
> session 1: commit
> session 2: reindex table test_toast ;
> and now we have two toast indexes. DROP INDEX is able to remove only invalid ones. Valid index gives "ERROR: permission denied: "pg_toast_16426_index_ccnew" is a system catalog"
> [1]: https://www.postgresql.org/message-id/CAB7nPqT%2B6igqbUb59y04NEgHoBeUGYteuUr89AKnLTFNdB8Hyw%40mail.g...
It looks like this was never addressed.
I noticed a ccnew toast index sitting around since October - what do I do with it ?
ts=# DROP INDEX pg_toast.pg_toast_463881620_index_ccnew;
ERROR: permission denied: "pg_toast_463881620_index_ccnew" is a system catalog
--
Justin
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-18 05:29 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 22+ messages in thread
From: Michael Paquier @ 2020-02-18 05:29 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Sergei Kornilov <sk@zsrv.org>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andreas Karlsson <andreas@proxel.se>; pgsql-hackers
On Sun, Feb 16, 2020 at 01:08:35PM -0600, Justin Pryzby wrote:
> Forking old, long thread:
> https://www.postgresql.org/message-id/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
> On Fri, Jan 04, 2019 at 03:18:06PM +0300, Sergei Kornilov wrote:
>> About reindex invalid indexes - i found one good question in archives [1]: how about toast indexes?
>> I check it now, i am able drop invalid toast index, but i can not drop reduntant valid index.
>> Reproduce:
>> session 1: begin; select from test_toast ... for update;
>> session 2: reindex table CONCURRENTLY test_toast ;
>> session 2: interrupt by ctrl+C
>> session 1: commit
>> session 2: reindex table test_toast ;
>> and now we have two toast indexes. DROP INDEX is able to remove
>> only invalid ones. Valid index gives "ERROR: permission denied:
>> "pg_toast_16426_index_ccnew" is a system catalog"
>> [1]: https://www.postgresql.org/message-id/CAB7nPqT%2B6igqbUb59y04NEgHoBeUGYteuUr89AKnLTFNdB8Hyw%40mail.g...
>
> It looks like this was never addressed.
On HEAD, this exact scenario leads to the presence of an old toast
index pg_toast.pg_toast_*_index_ccold, causing the index to be skipped
on a follow-up concurrent reindex:
=# reindex table CONCURRENTLY test_toast ;
WARNING: XX002: cannot reindex invalid index
"pg_toast.pg_toast_16385_index_ccold" concurrently, skipping
LOCATION: ReindexRelationConcurrently, indexcmds.c:2863
REINDEX
And this toast index can be dropped while it remains invalid:
=# drop index pg_toast.pg_toast_16385_index_ccold;
DROP INDEX
I recall testing that stuff for all the interrupts which could be
triggered and in this case, this waits at step 5 within
WaitForLockersMultiple(). Now, in your case you take an extra step
with a plain REINDEX, which forces a rebuild of the invalid toast
index, making it per se valid, and not droppable.
Hmm. There could be an argument here for skipping invalid toast
indexes within reindex_index(), because we are sure about having at
least one valid toast index at anytime, and these are not concerned
with CIC.
Any thoughts?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200218052933.GH4176@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-18 06:06 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-02-18 06:06 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Sun, Feb 16, 2020 at 01:08:35PM -0600, Justin Pryzby wrote:
> > Forking old, long thread:
> > https://www.postgresql.org/message-id/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
> > On Fri, Jan 04, 2019 at 03:18:06PM +0300, Sergei Kornilov wrote:
> >> About reindex invalid indexes - i found one good question in archives [1]: how about toast indexes?
> >> I check it now, i am able drop invalid toast index, but i can not drop reduntant valid index.
> >> Reproduce:
> >> session 1: begin; select from test_toast ... for update;
> >> session 2: reindex table CONCURRENTLY test_toast ;
> >> session 2: interrupt by ctrl+C
> >> session 1: commit
> >> session 2: reindex table test_toast ;
> >> and now we have two toast indexes. DROP INDEX is able to remove
> >> only invalid ones. Valid index gives "ERROR: permission denied:
> >> "pg_toast_16426_index_ccnew" is a system catalog"
> >> [1]: https://www.postgresql.org/message-id/CAB7nPqT%2B6igqbUb59y04NEgHoBeUGYteuUr89AKnLTFNdB8Hyw%40mail.g...
> >
> > It looks like this was never addressed.
>
> On HEAD, this exact scenario leads to the presence of an old toast
> index pg_toast.pg_toast_*_index_ccold, causing the index to be skipped
> on a follow-up concurrent reindex:
> =# reindex table CONCURRENTLY test_toast ;
> WARNING: XX002: cannot reindex invalid index
> "pg_toast.pg_toast_16385_index_ccold" concurrently, skipping
> LOCATION: ReindexRelationConcurrently, indexcmds.c:2863
> REINDEX
>
> And this toast index can be dropped while it remains invalid:
> =# drop index pg_toast.pg_toast_16385_index_ccold;
> DROP INDEX
>
> I recall testing that stuff for all the interrupts which could be
> triggered and in this case, this waits at step 5 within
> WaitForLockersMultiple(). Now, in your case you take an extra step
> with a plain REINDEX, which forces a rebuild of the invalid toast
> index, making it per se valid, and not droppable.
>
> Hmm. There could be an argument here for skipping invalid toast
> indexes within reindex_index(), because we are sure about having at
> least one valid toast index at anytime, and these are not concerned
> with CIC.
Or even automatically drop any invalid index on toast relation in
reindex_relation, since those can't be due to a failed CIC?
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-18 06:19 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-02-18 06:19 UTC (permalink / raw)
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
On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm. There could be an argument here for skipping invalid toast
>> indexes within reindex_index(), because we are sure about having at
>> least one valid toast index at anytime, and these are not concerned
>> with CIC.
>
> Or even automatically drop any invalid index on toast relation in
> reindex_relation, since those can't be due to a failed CIC?
No, I don't like much outsmarting REINDEX with more index drops than
it needs to do. And this would not take care of the case with REINDEX
INDEX done directly on a toast index.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200218061913.GJ4176@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-18 06:39 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-02-18 06:39 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Tue, Feb 18, 2020 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> > On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> Hmm. There could be an argument here for skipping invalid toast
> >> indexes within reindex_index(), because we are sure about having at
> >> least one valid toast index at anytime, and these are not concerned
> >> with CIC.
> >
> > Or even automatically drop any invalid index on toast relation in
> > reindex_relation, since those can't be due to a failed CIC?
>
> No, I don't like much outsmarting REINDEX with more index drops than
> it needs to do. And this would not take care of the case with REINDEX
> INDEX done directly on a toast index.
Well, we could still do both but I get the objection. Then skipping
invalid toast indexes in reindex_relation looks like the best fix.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-22 07:09 Julien Rouhaud <rjuju123@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-02-22 07:09 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Tue, Feb 18, 2020 at 07:39:49AM +0100, Julien Rouhaud wrote:
> On Tue, Feb 18, 2020 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> > > On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >> Hmm. There could be an argument here for skipping invalid toast
> > >> indexes within reindex_index(), because we are sure about having at
> > >> least one valid toast index at anytime, and these are not concerned
> > >> with CIC.
> > >
> > > Or even automatically drop any invalid index on toast relation in
> > > reindex_relation, since those can't be due to a failed CIC?
> >
> > No, I don't like much outsmarting REINDEX with more index drops than
> > it needs to do. And this would not take care of the case with REINDEX
> > INDEX done directly on a toast index.
>
> Well, we could still do both but I get the objection. Then skipping
> invalid toast indexes in reindex_relation looks like the best fix.
PFA a patch to fix the problem using this approach.
I also added isolation tester regression tests. The failure is simulated using
a pg_cancel_backend() on top of pg_stat_activity, using filters on a
specifically set application name and the query text to avoid any unwanted
interaction. I also added a 1s locking delay, to ensure that even slow/CCA
machines can consistently reproduce the failure. Maybe that's not enough, or
maybe testing this scenario is not worth the extra time.
From 990d265e5d576b3b4133232f302d6207987f1511 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 29 +++++++++++
.../expected/reindex-concurrently.out | 49 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 23 +++++++++
3 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(NOTICE,
+ (errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..fa9039c125 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,50 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: lock timeout reindex unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+step timeout: SET statement_timeout to 5000;
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step unlock: SELECT pg_sleep(6); COMMIT;
+pg_sleep
+
+
+step reindex: <... completed>
+error in steps unlock reindex: ERROR: canceling statement due to statement timeout
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..0599e8e213 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
session "s3"
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+step "timeout" { SET statement_timeout to 5000; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { SELECT pg_sleep(6); COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# 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.
+permutation "lock" "timeout" "reindex" "unlock" "check_invalid"
+ "normal_reindex" "check_invalid"
+ "nowarn" "reindex" "check_invalid"
--
2.20.1
Attachments:
[text/plain] 0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v1.patch (6.3K, ../../20200222070924.GA57285@nol/2-0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v1.patch)
download | inline diff:
From 990d265e5d576b3b4133232f302d6207987f1511 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 29 +++++++++++
.../expected/reindex-concurrently.out | 49 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 23 +++++++++
3 files changed, 100 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(NOTICE,
+ (errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..fa9039c125 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,50 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: lock timeout reindex unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+step timeout: SET statement_timeout to 5000;
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step unlock: SELECT pg_sleep(6); COMMIT;
+pg_sleep
+
+
+step reindex: <... completed>
+error in steps unlock reindex: ERROR: canceling statement due to statement timeout
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..0599e8e213 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -31,6 +31,22 @@ step "end2" { COMMIT; }
session "s3"
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+step "timeout" { SET statement_timeout to 5000; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "unlock" { SELECT pg_sleep(6); COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +54,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# 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.
+permutation "lock" "timeout" "reindex" "unlock" "check_invalid"
+ "normal_reindex" "check_invalid"
+ "nowarn" "reindex" "check_invalid"
--
2.20.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-22 11:13 Justin Pryzby <pryzby@telsasoft.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 22+ messages in thread
From: Justin Pryzby @ 2020-02-22 11:13 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Sergei Kornilov <sk@zsrv.org>; Peter Eisentraut <peter.eisentraut@2ndquadrant.com>; Michael Paquier <michael.paquier@gmail.com>; Andreas Karlsson <andreas@proxel.se>; pgsql-hackers
On Tue, Feb 18, 2020 at 02:29:33PM +0900, Michael Paquier wrote:
> On Sun, Feb 16, 2020 at 01:08:35PM -0600, Justin Pryzby wrote:
> > Forking old, long thread:
> > https://www.postgresql.org/message-id/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
> > On Fri, Jan 04, 2019 at 03:18:06PM +0300, Sergei Kornilov wrote:
> >> About reindex invalid indexes - i found one good question in archives [1]: how about toast indexes?
> >> I check it now, i am able drop invalid toast index, but i can not drop reduntant valid index.
> >> Reproduce:
> >> session 1: begin; select from test_toast ... for update;
> >> session 2: reindex table CONCURRENTLY test_toast ;
> >> session 2: interrupt by ctrl+C
> >> session 1: commit
> >> session 2: reindex table test_toast ;
> >> and now we have two toast indexes. DROP INDEX is able to remove
> >> only invalid ones. Valid index gives "ERROR: permission denied:
> >> "pg_toast_16426_index_ccnew" is a system catalog"
> >> [1]: https://www.postgresql.org/message-id/CAB7nPqT%2B6igqbUb59y04NEgHoBeUGYteuUr89AKnLTFNdB8Hyw%40mail.g...
> >
> > It looks like this was never addressed.
>
> On HEAD, this exact scenario leads to the presence of an old toast
> index pg_toast.pg_toast_*_index_ccold, causing the index to be skipped
> on a follow-up concurrent reindex:
> =# reindex table CONCURRENTLY test_toast ;
> WARNING: XX002: cannot reindex invalid index
> "pg_toast.pg_toast_16385_index_ccold" concurrently, skipping
> LOCATION: ReindexRelationConcurrently, indexcmds.c:2863
> REINDEX
>
> And this toast index can be dropped while it remains invalid:
> =# drop index pg_toast.pg_toast_16385_index_ccold;
> DROP INDEX
>
> I recall testing that stuff for all the interrupts which could be
> triggered and in this case, this waits at step 5 within
> WaitForLockersMultiple(). Now, in your case you take an extra step
> with a plain REINDEX, which forces a rebuild of the invalid toast
> index, making it per se valid, and not droppable.
>
> Hmm. There could be an argument here for skipping invalid toast
> indexes within reindex_index(), because we are sure about having at
> least one valid toast index at anytime, and these are not concerned
> with CIC.
Julien sent a patch for that, but here are my ideas (which you are free to
reject):
Could you require an AEL for that case, or something which will preclude
reindex table test_toast from working ?
Could you use atomic updates to ensure that exactly one index in an {old,new}
pair is invalid at any given time ?
Could you make the new (invalid) toast index not visible to other transactions?
--
Justin Pryzby
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-22 15:06 Julien Rouhaud <rjuju123@gmail.com>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-02-22 15:06 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Sat, Feb 22, 2020 at 08:09:24AM +0100, Julien Rouhaud wrote:
> On Tue, Feb 18, 2020 at 07:39:49AM +0100, Julien Rouhaud wrote:
> > On Tue, Feb 18, 2020 at 7:19 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Tue, Feb 18, 2020 at 07:06:25AM +0100, Julien Rouhaud wrote:
> > > > On Tue, Feb 18, 2020 at 6:30 AM Michael Paquier <michael@paquier.xyz> wrote:
> > > >> Hmm. There could be an argument here for skipping invalid toast
> > > >> indexes within reindex_index(), because we are sure about having at
> > > >> least one valid toast index at anytime, and these are not concerned
> > > >> with CIC.
>
> PFA a patch to fix the problem using this approach.
>
> I also added isolation tester regression tests. The failure is simulated using
> a pg_cancel_backend() on top of pg_stat_activity, using filters on a
> specifically set application name and the query text to avoid any unwanted
> interaction. I also added a 1s locking delay, to ensure that even slow/CCA
> machines can consistently reproduce the failure. Maybe that's not enough, or
> maybe testing this scenario is not worth the extra time.
Sorry, I just realized that I forgot to commit the last changes before sending
the patch, so here's the correct v2.
From 1b1bd50e4af4a2034638898129e6e49e3f4999da Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 29 ++++++++++
.../expected/reindex-concurrently.out | 55 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 27 +++++++++
3 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(NOTICE,
+ (errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..012b4874dd 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,56 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: lock reindex sleep kill unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step sleep: SELECT pg_sleep(1);
+pg_sleep
+
+
+step kill: SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+ WHERE application_name = 's3_reindex_concurrently'
+ AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;'
+pg_cancel_backend
+
+t
+step reindex: <... completed>
+error in steps kill reindex: ERROR: canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..ecd784cc4a 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -30,7 +30,27 @@ step "del2" { DELETE FROM reind_con_tab WHERE data = 'cccc'; }
step "end2" { COMMIT; }
session "s3"
+setup { SET application_name TO "s3_reindex_concurrently"; }
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "sleep" { SELECT pg_sleep(1); }
+step "kill" { SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+ WHERE application_name = 's3_reindex_concurrently'
+ AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;' }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +58,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# 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.
+permutation "lock" "reindex" "sleep" "kill" "unlock" "check_invalid"
+ "normal_reindex" "check_invalid"
+ "nowarn" "reindex" "check_invalid"
--
2.20.1
Attachments:
[text/plain] 0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v2.patch (6.8K, ../../20200222150657.GA54846@nol/2-0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v2.patch)
download | inline diff:
From 1b1bd50e4af4a2034638898129e6e49e3f4999da Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by:
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 29 ++++++++++
.../expected/reindex-concurrently.out | 55 ++++++++++++++++++-
.../isolation/specs/reindex-concurrently.spec | 27 +++++++++
3 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..201a3c39df 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3717,6 +3718,34 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(NOTICE,
+ (errmsg("skipping invalid index \"%s.%s\"",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/test/isolation/expected/reindex-concurrently.out b/src/test/isolation/expected/reindex-concurrently.out
index 9e04169b2f..012b4874dd 100644
--- a/src/test/isolation/expected/reindex-concurrently.out
+++ b/src/test/isolation/expected/reindex-concurrently.out
@@ -1,4 +1,4 @@
-Parsed test spec with 3 sessions
+Parsed test spec with 5 sessions
starting permutation: reindex sel1 upd2 ins2 del2 end1 end2
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
@@ -76,3 +76,56 @@ step end1: COMMIT;
step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
step end2: COMMIT;
step reindex: <... completed>
+
+starting permutation: lock reindex sleep kill unlock check_invalid normal_reindex check_invalid nowarn reindex check_invalid
+step lock: BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE;
+data
+
+aa
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab; <waiting ...>
+step sleep: SELECT pg_sleep(1);
+pg_sleep
+
+
+step kill: SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+ WHERE application_name = 's3_reindex_concurrently'
+ AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;'
+pg_cancel_backend
+
+t
+step reindex: <... completed>
+error in steps kill reindex: ERROR: canceling statement due to user request
+step unlock: COMMIT;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step normal_reindex: REINDEX TABLE reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
+step nowarn: SET client_min_messages = 'ERROR';
+step reindex: REINDEX TABLE CONCURRENTLY reind_con_tab;
+step check_invalid: SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C";
+indisvalid
+
+f
+t
diff --git a/src/test/isolation/specs/reindex-concurrently.spec b/src/test/isolation/specs/reindex-concurrently.spec
index eb59fe0cba..ecd784cc4a 100644
--- a/src/test/isolation/specs/reindex-concurrently.spec
+++ b/src/test/isolation/specs/reindex-concurrently.spec
@@ -30,7 +30,27 @@ step "del2" { DELETE FROM reind_con_tab WHERE data = 'cccc'; }
step "end2" { COMMIT; }
session "s3"
+setup { SET application_name TO "s3_reindex_concurrently"; }
step "reindex" { REINDEX TABLE CONCURRENTLY reind_con_tab; }
+step "nowarn" { SET client_min_messages = 'ERROR'; }
+
+session "s4"
+step "lock" { BEGIN; SELECT data FROM reind_con_tab WHERE data = 'aa' FOR UPDATE; }
+step "sleep" { SELECT pg_sleep(1); }
+step "kill" { SELECT pg_cancel_backend(pid) FROM pg_stat_activity
+ WHERE application_name = 's3_reindex_concurrently'
+ AND query = 'REINDEX TABLE CONCURRENTLY reind_con_tab;' }
+step "unlock" { COMMIT; }
+
+session "s5"
+setup { SET client_min_messages = 'WARNING'; }
+step "normal_reindex" { REINDEX TABLE reind_con_tab; }
+step "check_invalid" {SELECT 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_tab'
+ ORDER BY i.indisvalid::text COLLATE "C"; }
permutation "reindex" "sel1" "upd2" "ins2" "del2" "end1" "end2"
permutation "sel1" "reindex" "upd2" "ins2" "del2" "end1" "end2"
@@ -38,3 +58,10 @@ permutation "sel1" "upd2" "reindex" "ins2" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "reindex" "del2" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "reindex" "end1" "end2"
permutation "sel1" "upd2" "ins2" "del2" "end1" "reindex" "end2"
+# A failed REINDEX CONCURRENTLY will leave an invalid index on reind_con_tab
+# 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.
+permutation "lock" "reindex" "sleep" "kill" "unlock" "check_invalid"
+ "normal_reindex" "check_invalid"
+ "nowarn" "reindex" "check_invalid"
--
2.20.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-27 07:32 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-02-27 07:32 UTC (permalink / raw)
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
On Sat, Feb 22, 2020 at 04:06:57PM +0100, Julien Rouhaud wrote:
> Sorry, I just realized that I forgot to commit the last changes before sending
> the patch, so here's the correct v2.
Thanks for the patch.
> + if (skipit)
> + {
> + ereport(NOTICE,
> + (errmsg("skipping invalid index \"%s.%s\"",
> + get_namespace_name(get_rel_namespace(indexOid)),
> + get_rel_name(indexOid))));
ReindexRelationConcurrently() issues a WARNING when bumping on an
invalid index, shouldn't the same log level be used?
Even with this patch, it is possible to reindex an invalid toast index
with REINDEX INDEX (with and without CONCURRENTLY), which is the
problem I mentioned upthread (Er, actually only for the non-concurrent
case as told about reindex_index). Shouldn't both cases be prevented
as well with an ERROR?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200227073211.GA403330@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-02-27 08:07 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-02-27 08:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Thu, Feb 27, 2020 at 04:32:11PM +0900, Michael Paquier wrote:
> On Sat, Feb 22, 2020 at 04:06:57PM +0100, Julien Rouhaud wrote:
> > Sorry, I just realized that I forgot to commit the last changes before sending
> > the patch, so here's the correct v2.
>
> Thanks for the patch.
>
> > + if (skipit)
> > + {
> > + ereport(NOTICE,
> > + (errmsg("skipping invalid index \"%s.%s\"",
> > + get_namespace_name(get_rel_namespace(indexOid)),
> > + get_rel_name(indexOid))));
>
> ReindexRelationConcurrently() issues a WARNING when bumping on an
> invalid index, shouldn't the same log level be used?
For ReindexRelationConcurrently, the index is skipped because the feature isn't
supported, thus a warning. In this case that would work, it's just that we
don't want to process such indexes, so I used a notice instead.
I'm not opposed to use a warning instead if you prefer. What errcode should be
used though, ERRCODE_WARNING? ERRCODE_FEATURE_NOT_SUPPORTED doesn't feel
right.
> Even with this patch, it is possible to reindex an invalid toast index
> with REINDEX INDEX (with and without CONCURRENTLY), which is the
> problem I mentioned upthread (Er, actually only for the non-concurrent
> case as told about reindex_index). Shouldn't both cases be prevented
> as well with an ERROR?
Ah indeed, sorry I missed that.
While looking at it, I see that invalid indexes seem to leaked when the table
is dropped, with no way to get rid of them:
s1:
CREATE TABLE t1(val text);
CREATE INDEX ON t1 (val);
BEGIN;
SELECT * FROM t1 FOR UPDATE;
s2:
REINDEX TABLE CONCURRENTLY t1;
[stucked and canceled]
SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
indexrelid | indrelid
-------------------------------------+-------------------------
t1_val_idx_ccold | t1
pg_toast.pg_toast_16385_index_ccold | pg_toast.pg_toast_16385
(2 rows)
s1:
ROLLBACK;
DROP TABLE t1;
SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
indexrelid | indrelid
-------------------------------------+----------
t1_val_idx_ccold | 16385
pg_toast.pg_toast_16385_index_ccold | 16388
(2 rows)
REINDEX INDEX t1_val_idx_ccold;
ERROR: XX000: could not open relation with OID 16385
LOCATION: relation_open, relation.c:62
DROP INDEX t1_val_idx_ccold;
ERROR: XX000: could not open relation with OID 16385
LOCATION: relation_open, relation.c:62
REINDEX INDEX pg_toast.pg_toast_16385_index_ccold;
ERROR: XX000: could not open relation with OID 16388
LOCATION: relation_open, relation.c:62
DROP INDEX pg_toast.pg_toast_16385_index_ccold;
ERROR: XX000: could not open relation with OID 16388
LOCATION: relation_open, relation.c:62
REINDEX DATABASE rjuju;
REINDEX
SELECT indexrelid::regclass, indrelid::regclass FROM pg_index WHERE NOT indisvalid;
indexrelid | indrelid
-------------------------------------+----------
t1_val_idx_ccold | 16385
pg_toast.pg_toast_16385_index_ccold | 16388
(2 rows)
Shouldn't DROP TABLE be fixed to also drop invalid indexes?
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-03 08:06 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-03 08:06 UTC (permalink / raw)
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
On Thu, Feb 27, 2020 at 09:07:35AM +0100, Julien Rouhaud wrote:
> While looking at it, I see that invalid indexes seem to leaked when the table
> is dropped, with no way to get rid of them:
>
> Shouldn't DROP TABLE be fixed to also drop invalid indexes?
Hmm. The problem here is that I think that we don't have the correct
correct interface to handle the dependency switching between the old
and new indexes from the start, and 68ac9cf made things better in some
aspects (like non-cancellation and old index drop) but not in others
(like yours, or even a column drop). changeDependenciesOf/On() have
been added especially for REINDEX CONCURRENTLY, but they are not
actually able to handle the case we want them to handle: do a switch
for both relations within the same scan. It is possible to use three
times the existing routines with a couple of CCIs in-between and what
I would call a fake placeholder OID to switch all the records cleanly,
but it would be actually cleaner to do a single scan of pg_depend and
switch the dependencies of both objects at once.
Attached is a draft patch to take care of that problem for HEAD. It
still needs a lot of polishing (variable names are not actually old
or new anymore, etc.) but that's enough to show the idea. If a version
reaches PG12, we would need to keep around the past routines to avoid
an ABI breakage, even if I doubt there are callers of it, but who
knows..
--
Michael
Attachments:
[text/x-diff] reindex-deps-v1.patch (7.4K, ../../20200303080642.GA1890@paquier.xyz/2-reindex-deps-v1.patch)
download | inline diff:
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 0cd6fcf027..5811d082b0 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -200,10 +200,10 @@ extern long changeDependencyFor(Oid classId, Oid objectId,
Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
-extern long changeDependenciesOf(Oid classId, Oid oldObjectId,
+extern long switchDependenciesOf(Oid classId, Oid oldObjectId,
Oid newObjectId);
-extern long changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+extern long switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId);
extern Oid getExtensionOfObject(Oid classId, Oid objectId);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..fb9bb276bb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,14 +1675,10 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
}
/*
- * Move all dependencies of and on the old index to the new one. First
- * remove any dependencies that the new index may have to provide an
- * initial clean state for the dependency switch, and then move all the
- * dependencies from the old index to the new one.
+ * Switch all dependencies of and on the old and new indexes.
*/
- deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
- changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
- changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+ switchDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
+ switchDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
/*
* Copy over statistics from old to new index
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index f9af245eec..11caa38083 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -396,7 +396,7 @@ changeDependencyFor(Oid classId, Oid objectId,
}
/*
- * Adjust all dependency records to come from a different object of the same type
+ * Switch dependency records between two objects of the same type
*
* classId/oldObjectId specify the old referencing object.
* newObjectId is the new referencing object (must be of class classId).
@@ -404,38 +404,41 @@ changeDependencyFor(Oid classId, Oid objectId,
* Returns the number of records updated.
*/
long
-changeDependenciesOf(Oid classId, Oid oldObjectId,
+switchDependenciesOf(Oid classId, Oid oldObjectId,
Oid newObjectId)
{
long count = 0;
Relation depRel;
- ScanKeyData key[2];
+ ScanKeyData key;
SysScanDesc scan;
HeapTuple tup;
depRel = table_open(DependRelationId, RowExclusiveLock);
- ScanKeyInit(&key[0],
+ ScanKeyInit(&key,
Anum_pg_depend_classid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(classId));
- ScanKeyInit(&key[1],
- Anum_pg_depend_objid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(oldObjectId));
scan = systable_beginscan(depRel, DependDependerIndexId, true,
- NULL, 2, key);
+ NULL, 1, &key);
while (HeapTupleIsValid((tup = systable_getnext(scan))))
{
Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
+ if (depform->objid != oldObjectId &&
+ depform->objid != newObjectId)
+ continue;
+
/* make a modifiable copy */
tup = heap_copytuple(tup);
depform = (Form_pg_depend) GETSTRUCT(tup);
- depform->objid = newObjectId;
+ if (depform->objid == oldObjectId)
+ depform->objid = newObjectId;
+ else
+ depform->objid = oldObjectId;
CatalogTupleUpdate(depRel, &tup->t_self, tup);
@@ -452,7 +455,7 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
}
/*
- * Adjust all dependency records to point to a different object of the same type
+ * Switch all dependency records between two objects of the same type.
*
* refClassId/oldRefObjectId specify the old referenced object.
* newRefObjectId is the new referenced object (must be of class refClassId).
@@ -460,74 +463,75 @@ changeDependenciesOf(Oid classId, Oid oldObjectId,
* Returns the number of records updated.
*/
long
-changeDependenciesOn(Oid refClassId, Oid oldRefObjectId,
+switchDependenciesOn(Oid refClassId, Oid oldRefObjectId,
Oid newRefObjectId)
{
long count = 0;
Relation depRel;
- ScanKeyData key[2];
+ ScanKeyData key;
SysScanDesc scan;
HeapTuple tup;
- ObjectAddress objAddr;
- bool newIsPinned;
+ ObjectAddress newObjAddr;
+ ObjectAddress oldObjAddr;
depRel = table_open(DependRelationId, RowExclusiveLock);
/*
- * If oldRefObjectId is pinned, there won't be any dependency entries on
- * it --- we can't cope in that case. (This isn't really worth expending
- * code to fix, in current usage; it just means you can't rename stuff out
- * of pg_catalog, which would likely be a bad move anyway.)
+ * If oldRefObjectId or newRefObjectId are pinned, there won't be any
+ * dependency entries on it --- we can't cope in that case. (This
+ * isn't really worth expending code to fix, in current usage; it
+ * just means you can't rename stuff out of pg_catalog, which would
+ * likely be a bad move anyway.)
*/
- objAddr.classId = refClassId;
- objAddr.objectId = oldRefObjectId;
- objAddr.objectSubId = 0;
+ oldObjAddr.classId = refClassId;
+ oldObjAddr.objectId = oldRefObjectId;
+ oldObjAddr.objectSubId = 0;
- if (isObjectPinned(&objAddr, depRel))
+ if (isObjectPinned(&oldObjAddr, depRel))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot remove dependency on %s because it is a system object",
- getObjectDescription(&objAddr))));
+ getObjectDescription(&oldObjAddr))));
- /*
- * We can handle adding a dependency on something pinned, though, since
- * that just means deleting the dependency entry.
- */
- objAddr.objectId = newRefObjectId;
+ newObjAddr.classId = refClassId;
+ newObjAddr.objectId = newRefObjectId;
+ newObjAddr.objectSubId = 0;
- newIsPinned = isObjectPinned(&objAddr, depRel);
+ if (isObjectPinned(&newObjAddr, depRel))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot remove dependency on %s because it is a system object",
+ getObjectDescription(&newObjAddr))));
/* Now search for dependency records */
- ScanKeyInit(&key[0],
+ ScanKeyInit(&key,
Anum_pg_depend_refclassid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(refClassId));
- ScanKeyInit(&key[1],
- Anum_pg_depend_refobjid,
- BTEqualStrategyNumber, F_OIDEQ,
- ObjectIdGetDatum(oldRefObjectId));
scan = systable_beginscan(depRel, DependReferenceIndexId, true,
- NULL, 2, key);
+ NULL, 1, &key);
while (HeapTupleIsValid((tup = systable_getnext(scan))))
{
Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup);
- if (newIsPinned)
- CatalogTupleDelete(depRel, &tup->t_self);
- else
- {
- /* make a modifiable copy */
- tup = heap_copytuple(tup);
- depform = (Form_pg_depend) GETSTRUCT(tup);
+ if (depform->refobjid != oldRefObjectId &&
+ depform->refobjid != newRefObjectId)
+ continue;
+ /* make a modifiable copy */
+ tup = heap_copytuple(tup);
+ depform = (Form_pg_depend) GETSTRUCT(tup);
+
+ if (depform->refobjid == oldRefObjectId)
depform->refobjid = newRefObjectId;
+ else
+ depform->refobjid = oldRefObjectId;
- CatalogTupleUpdate(depRel, &tup->t_self, tup);
+ CatalogTupleUpdate(depRel, &tup->t_self, tup);
- heap_freetuple(tup);
- }
+ heap_freetuple(tup);
count++;
}
[application/pgp-signature] signature.asc (832B, ../../20200303080642.GA1890@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-03 09:25 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-03 09:25 UTC (permalink / raw)
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
On Tue, Mar 03, 2020 at 05:06:42PM +0900, Michael Paquier wrote:
> Attached is a draft patch to take care of that problem for HEAD. It
> still needs a lot of polishing (variable names are not actually old
> or new anymore, etc.) but that's enough to show the idea. If a version
> reaches PG12, we would need to keep around the past routines to avoid
> an ABI breakage, even if I doubt there are callers of it, but who
> knows..
Or actually, a more simple solution is to abuse of the two existing
routines so as the dependency switch is done the other way around,
from the new index to the old one. That would visibly work because
there is no CCI between each scan, and that's faster because the scan
of pg_depend is done only on the entries in need of an update. I'll
look at that again tomorrow, it is late here and I may be missing
something obvious.
--
Michael
Attachments:
[text/x-diff] reindex-deps-v2.patch (1.0K, ../../20200303092551.GB1890@paquier.xyz/2-reindex-deps-v2.patch)
download | inline diff:
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..736bc9f66c 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,14 +1675,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
}
/*
- * Move all dependencies of and on the old index to the new one. First
- * remove any dependencies that the new index may have to provide an
- * initial clean state for the dependency switch, and then move all the
- * dependencies from the old index to the new one.
+ * Swap all dependencies of and on the old index to the new one, and
+ * vice-versa.
*/
- deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
+ changeDependenciesOf(RelationRelationId, newIndexId, oldIndexId);
+ changeDependenciesOn(RelationRelationId, newIndexId, oldIndexId);
/*
* Copy over statistics from old to new index
[application/pgp-signature] signature.asc (832B, ../../20200303092551.GB1890@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-04 05:15 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-04 05:15 UTC (permalink / raw)
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
On Tue, Mar 03, 2020 at 06:25:51PM +0900, Michael Paquier wrote:
> Or actually, a more simple solution is to abuse of the two existing
> routines so as the dependency switch is done the other way around,
> from the new index to the old one. That would visibly work because
> there is no CCI between each scan, and that's faster because the scan
> of pg_depend is done only on the entries in need of an update. I'll
> look at that again tomorrow, it is late here and I may be missing
> something obvious.
It was a good inspiration. I have been torturing this patch today and
played with it by injecting elog(ERROR) calls in the middle of reindex
concurrently for all the phases, and checked manually the handling of
entries in pg_depend for the new and old indexes, and these correctly
map. So this is taking care of your problem. Attached is an updated
patch with an updated comment about the dependency of this code with
CCIs. I'd like to go fix this issue first.
--
Michael
Attachments:
[text/x-diff] reindex-deps-v3.patch (1.1K, ../../20200304051510.GE2593@paquier.xyz/2-reindex-deps-v3.patch)
download | inline diff:
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1681f61727..7223679033 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1675,12 +1675,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
}
/*
- * Move all dependencies of and on the old index to the new one. First
- * remove any dependencies that the new index may have to provide an
- * initial clean state for the dependency switch, and then move all the
- * dependencies from the old index to the new one.
+ * Swap all dependencies of and on the old index to the new one, and
+ * vice-versa. Note that a call to CommandCounterIncrement() would cause
+ * duplicate entries in pg_depend, so this should not be done.
*/
- deleteDependencyRecordsFor(RelationRelationId, newIndexId, false);
+ changeDependenciesOf(RelationRelationId, newIndexId, oldIndexId);
+ changeDependenciesOn(RelationRelationId, newIndexId, oldIndexId);
+
changeDependenciesOf(RelationRelationId, oldIndexId, newIndexId);
changeDependenciesOn(RelationRelationId, oldIndexId, newIndexId);
[application/pgp-signature] signature.asc (832B, ../../20200304051510.GE2593@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-04 08:21 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-03-04 08:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Wed, Mar 4, 2020 at 6:15 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Mar 03, 2020 at 06:25:51PM +0900, Michael Paquier wrote:
> > Or actually, a more simple solution is to abuse of the two existing
> > routines so as the dependency switch is done the other way around,
> > from the new index to the old one. That would visibly work because
> > there is no CCI between each scan, and that's faster because the scan
> > of pg_depend is done only on the entries in need of an update. I'll
> > look at that again tomorrow, it is late here and I may be missing
> > something obvious.
>
> It was a good inspiration. I have been torturing this patch today and
> played with it by injecting elog(ERROR) calls in the middle of reindex
> concurrently for all the phases, and checked manually the handling of
> entries in pg_depend for the new and old indexes, and these correctly
> map. So this is taking care of your problem. Attached is an updated
> patch with an updated comment about the dependency of this code with
> CCIs. I'd like to go fix this issue first.
Thanks for the patch! I started to look at it during the weekend, but
I got interrupted and unfortunately didn't had time to look at it
since.
The fix looks good to me. I also tried multiple failure scenario and
it's unsurprisingly working just fine. Should we add some regression
tests for that? I guess most of it could be borrowed from the patch
to fix the toast index issue I sent last week.
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-05 03:53 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-05 03:53 UTC (permalink / raw)
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
On Wed, Mar 04, 2020 at 09:21:45AM +0100, Julien Rouhaud wrote:
> Thanks for the patch! I started to look at it during the weekend, but
> I got interrupted and unfortunately didn't had time to look at it
> since.
No problem, thanks for looking at it. I have looked at it again this
morning, and applied it.
> The fix looks good to me. I also tried multiple failure scenario and
> it's unsurprisingly working just fine. Should we add some regression
> tests for that? I guess most of it could be borrowed from the patch
> to fix the toast index issue I sent last week.
I have doubts when it comes to use a strategy based on
pg_cancel_backend() and a match of application_name (see for example
5ad72ce but I cannot find the associated thread). I think that we
could design something more robust here and usable by all tests, with
two things coming into my mind:
- A new meta-command for isolation tests to be able to cancel a
session with PQcancel().
- Fault injection in the backend.
For the case of this thread, the cancellation command would be a better
match.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200305035354.GQ2593@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-05 16:57 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-03-05 16:57 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Thu, Mar 05, 2020 at 12:53:54PM +0900, Michael Paquier wrote:
> On Wed, Mar 04, 2020 at 09:21:45AM +0100, Julien Rouhaud wrote:
> > Thanks for the patch! I started to look at it during the weekend, but
> > I got interrupted and unfortunately didn't had time to look at it
> > since.
>
> No problem, thanks for looking at it. I have looked at it again this
> morning, and applied it.
>
> > The fix looks good to me. I also tried multiple failure scenario and
> > it's unsurprisingly working just fine. Should we add some regression
> > tests for that? I guess most of it could be borrowed from the patch
> > to fix the toast index issue I sent last week.
>
> I have doubts when it comes to use a strategy based on
> pg_cancel_backend() and a match of application_name (see for example
> 5ad72ce but I cannot find the associated thread). I think that we
> could design something more robust here and usable by all tests, with
> two things coming into my mind:
> - A new meta-command for isolation tests to be able to cancel a
> session with PQcancel().
> - Fault injection in the backend.
> For the case of this thread, the cancellation command would be a better
> match.
I agree that the approach wasn't quite robust. I'll try to look at adding a
new command for isolationtester, but that's probably not something we want to
put in pg13?
Here's a v3 that takes address the various comments you previously noted, and
for which I also removed the regression tests.
Note that while looking at it, I noticed another bug in RIC:
# create table t1(id integer, val text); create index on t1(val);
CREATE TABLE
CREATE INDEX
# reindex table concurrently t1;
^CCancel request sent
ERROR: 57014: canceling statement due to user request
LOCATION: ProcessInterrupts, postgres.c:3171
# select indexrelid::regclass, indrelid::regclass, indexrelid, indrelid from pg_index where not indisvalid;
indexrelid | indrelid | indexrelid | indrelid
-------------------------------------+-------------------------+------------+----------
t1_val_idx_ccold | t1 | 16401 | 16395
pg_toast.pg_toast_16395_index_ccold | pg_toast.pg_toast_16395 | 16400 | 16398
(2 rows)
# reindex table concurrently t1;
WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
LOCATION: ReindexRelationConcurrently, indexcmds.c:2821
WARNING: XX002: cannot reindex invalid index "pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
LOCATION: ReindexRelationConcurrently, indexcmds.c:2867
REINDEX
# reindex index concurrently t1_val_idx_ccold;
REINDEX
That case is also fixed in this patch.
From 77ec865a9a655b2b973846f9a8fa93c966ca55f5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++
src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++-------
2 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ec20ba38d1..7985d98aa8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2337,6 +2338,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
Oid indOid;
Relation irel;
char persistence;
+ bool invalidtoastindex;
/*
* Find and lock index, and check permissions on table; use callback to
@@ -2362,6 +2364,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relkind == RELKIND_INDEX &&
+ !irel->rd_index->indisvalid);
+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2371,6 +2376,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
+ if (invalidtoastindex)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ return;
+ }
+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
@@ -2890,25 +2905,37 @@ ReindexRelationConcurrently(Oid relationOid, int options)
case RELKIND_INDEX:
{
Oid heapId = IndexGetRelation(relationOid, false);
+ Relation indexRelation = index_open(relationOid,
+ ShareUpdateExclusiveLock);
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs concurrently")));
+ else if (!indexRelation->rd_index->indisvalid)
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
+ get_namespace_name(get_rel_namespace(relationOid)),
+ get_rel_name(relationOid))));
+ else
+ {
+ /* Save the list of relation OIDs in private context */
+ oldcontext = MemoryContextSwitchTo(private_context);
- /* 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);
- /* Track the heap relation of this index for session locks */
- heapRelationIds = list_make1_oid(heapId);
+ /*
+ * Save the list of relation OIDs in private context. Note
+ * that invalid indexes are allowed here.
+ */
+ indexIds = lappend_oid(indexIds, relationOid);
- /*
- * Save the list of relation OIDs in private context. Note
- * that invalid indexes are allowed here.
- */
- indexIds = lappend_oid(indexIds, relationOid);
+ MemoryContextSwitchTo(oldcontext);
+ }
- MemoryContextSwitchTo(oldcontext);
+ index_close(indexRelation, NoLock);
break;
}
case RELKIND_PARTITIONED_TABLE:
--
2.20.1
Attachments:
[text/plain] 0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v3.patch (5.7K, ../../20200305165707.GA35281@nol/2-0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v3.patch)
download | inline diff:
From 77ec865a9a655b2b973846f9a8fa93c966ca55f5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++
src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++-------
2 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ec20ba38d1..7985d98aa8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2337,6 +2338,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
Oid indOid;
Relation irel;
char persistence;
+ bool invalidtoastindex;
/*
* Find and lock index, and check permissions on table; use callback to
@@ -2362,6 +2364,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relkind == RELKIND_INDEX &&
+ !irel->rd_index->indisvalid);
+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2371,6 +2376,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
+ if (invalidtoastindex)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ return;
+ }
+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
@@ -2890,25 +2905,37 @@ ReindexRelationConcurrently(Oid relationOid, int options)
case RELKIND_INDEX:
{
Oid heapId = IndexGetRelation(relationOid, false);
+ Relation indexRelation = index_open(relationOid,
+ ShareUpdateExclusiveLock);
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs concurrently")));
+ else if (!indexRelation->rd_index->indisvalid)
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid index \"%s.%s\" concurrently, skipping",
+ get_namespace_name(get_rel_namespace(relationOid)),
+ get_rel_name(relationOid))));
+ else
+ {
+ /* Save the list of relation OIDs in private context */
+ oldcontext = MemoryContextSwitchTo(private_context);
- /* 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);
- /* Track the heap relation of this index for session locks */
- heapRelationIds = list_make1_oid(heapId);
+ /*
+ * Save the list of relation OIDs in private context. Note
+ * that invalid indexes are allowed here.
+ */
+ indexIds = lappend_oid(indexIds, relationOid);
- /*
- * Save the list of relation OIDs in private context. Note
- * that invalid indexes are allowed here.
- */
- indexIds = lappend_oid(indexIds, relationOid);
+ MemoryContextSwitchTo(oldcontext);
+ }
- MemoryContextSwitchTo(oldcontext);
+ index_close(indexRelation, NoLock);
break;
}
case RELKIND_PARTITIONED_TABLE:
--
2.20.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-06 01:38 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-06 01:38 UTC (permalink / raw)
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
On Thu, Mar 05, 2020 at 05:57:07PM +0100, Julien Rouhaud wrote:
> I agree that the approach wasn't quite robust. I'll try to look at adding a
> new command for isolationtester, but that's probably not something we want to
> put in pg13?
Yes, that's too late.
> Note that while looking at it, I noticed another bug in RIC:
>
> [...]
>
> # reindex table concurrently t1;
> WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
> LOCATION: ReindexRelationConcurrently, indexcmds.c:2821
> WARNING: XX002: cannot reindex invalid index "pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
> LOCATION: ReindexRelationConcurrently, indexcmds.c:2867
> REINDEX
> # reindex index concurrently t1_val_idx_ccold;
> REINDEX
>
> That case is also fixed in this patch.
This choice is intentional. The idea about bypassing invalid indexes
for table-level REINDEX is that this would lead to a bloat in the
number of relations to handling if multiple runs are failing, leading
to more and more invalid indexes to handle each time. Allowing a
single invalid non-toast index to be reindexed with CONCURRENTLY can
be helpful in some cases, like for example a CIC for a unique index
that failed and was invalid, where the relation already defined can be
reused.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200306013844.GE52814@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-06 12:36 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-03-06 12:36 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Fri, Mar 06, 2020 at 10:38:44AM +0900, Michael Paquier wrote:
> On Thu, Mar 05, 2020 at 05:57:07PM +0100, Julien Rouhaud wrote:
> > I agree that the approach wasn't quite robust. I'll try to look at adding a
> > new command for isolationtester, but that's probably not something we want to
> > put in pg13?
>
> Yes, that's too late.
>
> > Note that while looking at it, I noticed another bug in RIC:
> >
> > [...]
> >
> > # reindex table concurrently t1;
> > WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
> > LOCATION: ReindexRelationConcurrently, indexcmds.c:2821
> > WARNING: XX002: cannot reindex invalid index "pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
> > LOCATION: ReindexRelationConcurrently, indexcmds.c:2867
> > REINDEX
> > # reindex index concurrently t1_val_idx_ccold;
> > REINDEX
> >
> > That case is also fixed in this patch.
>
> This choice is intentional. The idea about bypassing invalid indexes
> for table-level REINDEX is that this would lead to a bloat in the
> number of relations to handling if multiple runs are failing, leading
> to more and more invalid indexes to handle each time. Allowing a
> single invalid non-toast index to be reindexed with CONCURRENTLY can
> be helpful in some cases, like for example a CIC for a unique index
> that failed and was invalid, where the relation already defined can be
> reused.
Ah I see, thanks for the clarification. I guess there's room for improvement
in the comments about that, since the ERRCODE_FEATURE_NOT_SUPPORTED usage is
quite misleading there.
v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
non-TOAST index anymore.
From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++++++++++++
src/backend/commands/indexcmds.c | 15 +++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3f3a89fe92..08d74fecd1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2309,6 +2310,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
Oid indOid;
Relation irel;
char persistence;
+ bool invalidtoastindex;
/*
* Find and lock index, and check permissions on table; use callback to
@@ -2334,6 +2336,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relnamespace == PG_TOAST_NAMESPACE &&
+ !irel->rd_index->indisvalid);
+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2343,6 +2348,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
+ if (invalidtoastindex)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ return;
+ }
+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
--
2.20.1
Attachments:
[text/plain] 0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v4.patch (4.0K, ../../20200306123648.GA49431@nol/2-0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v4.patch)
download | inline diff:
From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++++++++++++
src/backend/commands/indexcmds.c | 15 +++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those can only be
+ * a duplicate leftover of a failed REINDEX CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+
+ skipit = ((Form_pg_index) GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3f3a89fe92..08d74fecd1 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2309,6 +2310,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
Oid indOid;
Relation irel;
char persistence;
+ bool invalidtoastindex;
/*
* Find and lock index, and check permissions on table; use callback to
@@ -2334,6 +2336,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relnamespace == PG_TOAST_NAMESPACE &&
+ !irel->rd_index->indisvalid);
+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2343,6 +2348,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
+ if (invalidtoastindex)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ return;
+ }
+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
--
2.20.1
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-09 05:52 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-09 05:52 UTC (permalink / raw)
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
On Fri, Mar 06, 2020 at 01:36:48PM +0100, Julien Rouhaud wrote:
> Ah I see, thanks for the clarification. I guess there's room for improvement
> in the comments about that, since the ERRCODE_FEATURE_NOT_SUPPORTED usage is
> quite misleading there.
>
> v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
> non-TOAST index anymore.
Thanks. The position of the error check in reindex_relation() is
correct, but as it opens a relation for the cache lookup let's invent
a new routine in lsyscache.c to grab pg_index.indisvalid. It is
possible to make use of this routine with all the other checks:
- WARNING for REINDEX TABLE (non-conurrent)
- ERROR for REINDEX INDEX (non-conurrent)
- ERROR for REINDEX INDEX CONCURRENTLY
(There is already a WARNING for REINDEX TABLE CONCURRENTLY.)
I did not find the addition of an error check in ReindexIndex()
consistent with the existing practice to check the state of the
relation reindexed in reindex_index() (for the non-concurrent case)
and ReindexRelationConcurrently() (for the concurrent case). Okay,
this leads to the introduction of two new ERROR messages related to
invalid toast indexes for the concurrent and the non-concurrent cases
when using REINDEX INDEX instead of one, but having two messages leads
to something much more consistent with the rest, and all checks remain
centralized in the same routines.
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.
For the check in reindex_relation, it is more consistent to check the
namespace of the index instead of the parent relation IMO (the
previous patch used "rel", which refers to the parent table). This
has in practice no consequence though.
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 :/
Any opinions?
--
Michael
Attachments:
[text/x-diff] v2-0001-Forbid-reindex-of-invalid-indexes-on-TOAST-tables.patch (5.7K, ../../20200309055231.GC96055@paquier.xyz/2-v2-0001-Forbid-reindex-of-invalid-indexes-on-TOAST-tables.patch)
download | inline diff:
From 2e808a2971fcaf160f6a1e6c9c80366ff053bccf Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 9 Mar 2020 14:43:33 +0900
Subject: [PATCH v2] Forbid reindex of invalid indexes on TOAST tables
Such indexes can only be duplicated leftovers of a previously failed
REINDEX CONCURRENTLY command, and a valid equivalent is guaranteed to
exist already. As we only allow the drop of invalid indexes on TOAST
tables, reindexing these would lead to useless duplicated indexes that
can't be dropped anymore.
Thanks to Justin Pryzby for reminding that this problem was reported
long ago, but it has never been addressed.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion: https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.GA21832@telsasoft.com
Backpatch-through: 12
---
src/include/utils/lsyscache.h | 1 +
src/backend/catalog/index.c | 28 ++++++++++++++++++++++++++++
src/backend/commands/indexcmds.c | 12 ++++++++++++
src/backend/utils/cache/lsyscache.c | 23 +++++++++++++++++++++++
4 files changed, 64 insertions(+)
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index f132d39458..131d10eab0 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -181,6 +181,7 @@ extern char *get_namespace_name_or_temp(Oid nspid);
extern Oid get_range_subtype(Oid rangeOid);
extern Oid get_range_collation(Oid rangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
+extern bool get_index_isvalid(Oid index_oid);
#define type_is_array(typid) (get_element_type(typid) != InvalidOid)
/* type_is_array_domain accepts both plain arrays and domains over arrays */
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..ec2d7dc9cb 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3474,6 +3475,17 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex temporary tables of other sessions")));
+ /*
+ * Don't allow reindex on an invalid toast index. This is a leftover from
+ * a failed REINDEX CONCURRENTLY, and if rebuilt it would not be possible
+ * to drop it anymore.
+ */
+ if (RelationGetNamespace(iRel) == PG_TOAST_NAMESPACE &&
+ !get_index_isvalid(indexId))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid toast index")));
+
/*
* Also check for active uses of the index in the current transaction; we
* don't want to reindex underneath an open indexscan.
@@ -3724,6 +3736,22 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * Skip any invalid indexes on a TOAST table. These can only be
+ * duplicate leftovers from a failed REINDEX CONCURRENTLY, and if
+ * rebuilt it it won't be possible to drop them anymore.
+ */
+ if (get_rel_namespace(indexOid) == PG_TOAST_NAMESPACE &&
+ !get_index_isvalid(indexOid))
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid toast index \"%s.%s\", skipping",
+ get_namespace_name(get_rel_namespace(indexOid)),
+ get_rel_name(indexOid))));
+ continue;
+ }
+
reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 3f3a89fe92..6f1dee8643 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2868,6 +2869,17 @@ ReindexRelationConcurrently(Oid relationOid, int options)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex system catalogs concurrently")));
+ /*
+ * Don't allow reindex for an invalid toast index, as if rebuild
+ * it would not be possible to drop it. Its valid equivalent also
+ * already exists.
+ */
+ if (get_rel_namespace(relationOid) == PG_TOAST_NAMESPACE &&
+ !get_index_isvalid(relationOid))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid toast index concurrently")));
+
/* Save the list of relation OIDs in private context */
oldcontext = MemoryContextSwitchTo(private_context);
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 3da90cb72a..400e7689fe 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3227,3 +3227,26 @@ get_index_column_opclass(Oid index_oid, int attno)
return opclass;
}
+
+/*
+ * get_index_isvalid
+ *
+ * Given the index OID, return pg_index.indisvalid.
+ */
+bool
+get_index_isvalid(Oid index_oid)
+{
+ bool isvalid;
+ HeapTuple tuple;
+ Form_pg_index rd_index;
+
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ rd_index = (Form_pg_index) GETSTRUCT(tuple);
+ isvalid = rd_index->indisvalid;
+ ReleaseSysCache(tuple);
+
+ return isvalid;
+}
--
2.25.1
[application/pgp-signature] signature.asc (832B, ../../20200309055231.GC96055@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-09 07:04 Julien Rouhaud <rjuju123@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 22+ messages in thread
From: Julien Rouhaud @ 2020-03-09 07:04 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +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
On Mon, Mar 09, 2020 at 02:52:31PM +0900, Michael Paquier wrote:
> On Fri, Mar 06, 2020 at 01:36:48PM +0100, Julien Rouhaud wrote:
> >
> > v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
> > non-TOAST index anymore.
>
> Thanks. The position of the error check in reindex_relation() is
> correct, but as it opens a relation for the cache lookup let's invent
> a new routine in lsyscache.c to grab pg_index.indisvalid. It is
> possible to make use of this routine with all the other checks:
> - WARNING for REINDEX TABLE (non-conurrent)
> - ERROR for REINDEX INDEX (non-conurrent)
> - ERROR for REINDEX INDEX CONCURRENTLY
> (There is already a WARNING for REINDEX TABLE CONCURRENTLY.)
>
> I did not find the addition of an error check in ReindexIndex()
> consistent with the existing practice to check the state of the
> relation reindexed in reindex_index() (for the non-concurrent case)
> and ReindexRelationConcurrently() (for the concurrent case). Okay,
> this leads to the introduction of two new ERROR messages related to
> invalid toast indexes for the concurrent and the non-concurrent cases
> when using REINDEX INDEX instead of one, but having two messages leads
> to something much more consistent with the rest, and all checks remain
> centralized in the same routines.
I wanted to go this way at first but hesitated and finally chose to add less
checks, so I'm fine with this approach, and patch looks good to me.
> 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.
> For the check in reindex_relation, it is more consistent to check the
> namespace of the index instead of the parent relation IMO (the
> previous patch used "rel", which refers to the parent table). This
> has in practice no consequence though.
Oops yes.
> 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 :(
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-10 03:09 Michael Paquier <michael@paquier.xyz>
parent: Julien Rouhaud <rjuju123@gmail.com>
0 siblings, 1 reply; 22+ messages in thread
From: Michael Paquier @ 2020-03-10 03:09 UTC (permalink / raw)
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
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.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
[application/pgp-signature] signature.asc (832B, ../../20200310030942.GD4369@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: reindex concurrently and two toast indexes
@ 2020-03-10 08:01 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 22+ messages in thread
From: Michael Paquier @ 2020-03-10 08:01 UTC (permalink / raw)
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
On Tue, Mar 10, 2020 at 12:09:42PM +0900, Michael Paquier wrote:
> On Mon, Mar 09, 2020 at 08:04:27AM +0100, Julien Rouhaud wrote:
>> Agreed.
>
> Thanks for checking the patch.
And applied as 61d7c7b. Regarding the isolation tests, let's
brainstorm on what we can do, but I am afraid that it is too late for
13.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200310080143.GH4369@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 22+ messages in thread
end of thread, other threads:[~2020-03-10 08:01 UTC | newest]
Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-16 19:08 reindex concurrently and two toast indexes Justin Pryzby <pryzby@telsasoft.com>
2020-02-18 05:29 ` Michael Paquier <michael@paquier.xyz>
2020-02-18 06:06 ` Julien Rouhaud <rjuju123@gmail.com>
2020-02-18 06:19 ` Michael Paquier <michael@paquier.xyz>
2020-02-18 06:39 ` Julien Rouhaud <rjuju123@gmail.com>
2020-02-22 07:09 ` Julien Rouhaud <rjuju123@gmail.com>
2020-02-22 15:06 ` Julien Rouhaud <rjuju123@gmail.com>
2020-02-27 07:32 ` Michael Paquier <michael@paquier.xyz>
2020-02-27 08:07 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-03 08:06 ` Michael Paquier <michael@paquier.xyz>
2020-03-03 09:25 ` Michael Paquier <michael@paquier.xyz>
2020-03-04 05:15 ` Michael Paquier <michael@paquier.xyz>
2020-03-04 08:21 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-05 03:53 ` Michael Paquier <michael@paquier.xyz>
2020-03-05 16:57 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-06 01:38 ` Michael Paquier <michael@paquier.xyz>
2020-03-06 12:36 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-09 05:52 ` Michael Paquier <michael@paquier.xyz>
2020-03-09 07:04 ` Julien Rouhaud <rjuju123@gmail.com>
2020-03-10 03:09 ` Michael Paquier <michael@paquier.xyz>
2020-03-10 08:01 ` Michael Paquier <michael@paquier.xyz>
2020-02-22 11:13 ` Justin Pryzby <pryzby@telsasoft.com>
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