pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedAssertion on create index concurrently
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2021-06-28 09a69f6e23 fd7bc10ab4 ce8949c4b6 38ca11adeb 5160d5bb16 21257a8f6d Add test for CREATE INDEX CONCURRENTLY with not-so-immutable predicate
2021-06-28 acb60edf02 e52f7cbec1 08acba558b 3c465883b8 d3d0cbeb62 Make index_set_state_flags() transactional
* Assertion on create index concurrently
@ 2021-06-26 18:48 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Lepikhov @ 2021-06-26 18:48 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
I found one annoying assertion in the CREATE INDEX CONCURRENTLY
operation. It will happen if you add into WHERE part of CREATE INDEX
CONCURRENTLY some function that will make transactional update (see an
example in the attached patch).
It looks like a hack, but it lead to assertion and restart of the
instance, that isn't good.
I suggest to perform error, not assertion in this case (see the patch).
This problem affects PG 9.6 - 13. The master branch was fixed 9 months
ago by commit 83158f7, that made this operation transactional, but still.
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Use-more-safe-procedure-to-check-a-non-transactional.patch (text/plain, 3.5K) download
From 2d658eb9830c78e875b08daa86c608d5f327b1f3 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Sat, 26 Jun 2021 19:54:25 +0300
Subject: [PATCH] Use more safe procedure to check a non-transactional
CONCURRENT operation on transactional updates.
---
src/backend/catalog/index.c | 8 ++++++--
src/test/regress/expected/create_index.out | 17 +++++++++++++++++
src/test/regress/sql/create_index.sql | 13 +++++++++++++
3 files changed, 36 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index b9eed8ec38..ca91a7c74f 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3395,8 +3395,12 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
HeapTuple indexTuple;
Form_pg_index indexForm;
- /* Assert that current xact hasn't done any transactional updates */
- Assert(GetTopTransactionIdIfAny() == InvalidTransactionId);
+ /* Check that current xact hasn't done any transactional updates */
+ if (GetTopTransactionIdIfAny() != InvalidTransactionId)
+ ereport(ERROR,
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("Some transactional updates were made during a concurrent index operation")));
+
/* Open pg_index and fetch a writable copy of the index's tuple */
pg_index = table_open(IndexRelationId, RowExclusiveLock);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 8e11c089b1..93103334d8 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -1393,6 +1393,23 @@ BEGIN;
CREATE INDEX CONCURRENTLY concur_index7 ON concur_heap(f1);
ERROR: CREATE INDEX CONCURRENTLY cannot run inside a transaction block
COMMIT;
+CREATE FUNCTION predicate() RETURNS bool IMMUTABLE LANGUAGE plpgsql AS $$
+BEGIN
+ EXECUTE 'SELECT txid_current()';
+ RETURN true;
+END; $$;
+-- You can't do any transactional updates during concurrent index build.
+CREATE INDEX CONCURRENTLY concur_index8 ON concur_heap (f1) WHERE predicate();
+ERROR: Some transactional updates were made during a concurrent index operation
+-- Index exists but invalid. It must be actualized after REINDEX TABLE command.
+SELECT relname,indisvalid FROM pg_class, pg_index
+ WHERE relname = 'concur_index8' AND oid = indexrelid;
+ relname | indisvalid
+---------------+------------
+ concur_index8 | f
+(1 row)
+
+DROP INDEX concur_index8;
-- But you can do a regular index build in a transaction
BEGIN;
CREATE INDEX std_index on concur_heap(f2);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index c0392f052d..71779e3dce 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -487,6 +487,19 @@ BEGIN;
CREATE INDEX CONCURRENTLY concur_index7 ON concur_heap(f1);
COMMIT;
+CREATE FUNCTION predicate() RETURNS bool IMMUTABLE LANGUAGE plpgsql AS $$
+BEGIN
+ EXECUTE 'SELECT txid_current()';
+ RETURN true;
+END; $$;
+
+-- You can't do any transactional updates during concurrent index build.
+CREATE INDEX CONCURRENTLY concur_index8 ON concur_heap (f1) WHERE predicate();
+-- Index exists but invalid. It must be actualized after REINDEX TABLE command.
+SELECT relname,indisvalid FROM pg_class, pg_index
+ WHERE relname = 'concur_index8' AND oid = indexrelid;
+DROP INDEX concur_index8;
+
-- But you can do a regular index build in a transaction
BEGIN;
CREATE INDEX std_index on concur_heap(f2);
--
2.31.1
* Re: Assertion on create index concurrently
@ 2021-06-27 01:35 Michael Paquier <michael@paquier.xyz>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2021-06-27 01:35 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: pgsql-bugs@lists.postgresql.org
On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:
> I found one annoying assertion in the CREATE INDEX CONCURRENTLY operation.
> It will happen if you add into WHERE part of CREATE INDEX CONCURRENTLY some
> function that will make transactional update (see an example in the attached
> patch).
>
> It looks like a hack, but it lead to assertion and restart of the instance,
> that isn't good.
> I suggest to perform error, not assertion in this case (see the patch).
>
> This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
> by commit 83158f7, that made this operation transactional, but still.
This looks like a good argument for back-patching 83158f7 all the way
down, as 813fb03 exists since 9.4. Any thoughts from others?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Assertion on create index concurrently
@ 2021-06-27 15:37 Tom Lane <tgl@sss.pgh.pa.us>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2021-06-27 15:37 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; pgsql-bugs@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:
>> This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
>> by commit 83158f7, that made this operation transactional, but still.
> This looks like a good argument for back-patching 83158f7 all the way
> down, as 813fb03 exists since 9.4. Any thoughts from others?
+1 ... 83158f7 has survived long enough to have reasonable confidence
in it.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Assertion on create index concurrently
@ 2021-06-27 19:33 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Lepikhov @ 2021-06-27 19:33 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs@lists.postgresql.org
On 27/6/21 18:37, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> On Sat, Jun 26, 2021 at 09:48:45PM +0300, Andrey Lepikhov wrote:
>>> This problem affects PG 9.6 - 13. The master branch was fixed 9 months ago
>>> by commit 83158f7, that made this operation transactional, but still.
>
>> This looks like a good argument for back-patching 83158f7 all the way
>> down, as 813fb03 exists since 9.4. Any thoughts from others?
>
> +1 ... 83158f7 has survived long enough to have reasonable confidence
> in it.
Technically, 83158f7 is trivial patch. I think, some efforts is required
only for PG9.6. I prepared a patch for this version.
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Back-patch-of-83158f7-for-PG9_6.patch (text/plain, 2.4K) download
From 1bcb60119f8de5f63165e5de51289156747041c2 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Sun, 27 Jun 2021 22:29:40 +0300
Subject: [PATCH] Back-patch of 83158f7 for PG9_6
---
src/backend/catalog/index.c | 23 ++++++++---------------
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index f1fe529981..35c44fa26a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3281,18 +3281,10 @@ validate_index_heapscan(Relation heapRelation,
* index_set_state_flags - adjust pg_index state flags
*
* This is used during CREATE/DROP INDEX CONCURRENTLY to adjust the pg_index
- * flags that denote the index's state. Because the update is not
- * transactional and will not roll back on error, this must only be used as
- * the last step in a transaction that has not made any transactional catalog
- * updates!
+ * flags that denote the index's state.
*
- * Note that heap_inplace_update does send a cache inval message for the
+ * Note that CatalogTupleUpdate() sends a cache invalidation message for the
* tuple, so other sessions will hear about the update as soon as we commit.
- *
- * NB: In releases prior to PostgreSQL 9.4, the use of a non-transactional
- * update here would have been unsafe; now that MVCC rules apply even for
- * system catalog scans, we could potentially use a transactional update here
- * instead.
*/
void
index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
@@ -3300,9 +3292,7 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
Relation pg_index;
HeapTuple indexTuple;
Form_pg_index indexForm;
-
- /* Assert that current xact hasn't done any transactional updates */
- Assert(GetTopTransactionIdIfAny() == InvalidTransactionId);
+ CatalogIndexState indstate;
/* Open pg_index and fetch a writable copy of the index's tuple */
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
@@ -3362,8 +3352,11 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
break;
}
- /* ... and write it back in-place */
- heap_inplace_update(pg_index, indexTuple);
+ /* ... and update it */
+ indstate = CatalogOpenIndexes(pg_index);
+ simple_heap_update(pg_index, &indexTuple->t_self, indexTuple);
+ CatalogIndexInsert(indstate, indexTuple);
+ CatalogCloseIndexes(indstate);
heap_close(pg_index, RowExclusiveLock);
}
--
2.31.1
* Re: Assertion on create index concurrently
@ 2021-06-28 02:19 Michael Paquier <michael@paquier.xyz>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2021-06-28 02:19 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
On Sun, Jun 27, 2021 at 10:33:44PM +0300, Andrey Lepikhov wrote:
> Technically, 83158f7 is trivial patch. I think, some efforts is required
> only for PG9.6. I prepared a patch for this version.
Thanks. This version had two problems:
- The comment on top of index_set_state_flags() was not updated, still
mentioning CatalogTupleUpdate().
- CatalogUpdateIndexes() does the job for the index updates, and using
it is more consistent with the other areas of index.c in
REL9_6_STABLE.
Just to keep a cleaner history, I have split your test case into a
separate commit, and applied that and the backpatch of 83158f7.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2021-06-28 02:19 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-06-26 18:48 Assertion on create index concurrently Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2021-06-27 01:35 ` Michael Paquier <michael@paquier.xyz>
2021-06-27 15:37 ` Tom Lane <tgl@sss.pgh.pa.us>
2021-06-27 19:33 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2021-06-28 02:19 ` Michael Paquier <michael@paquier.xyz>
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