Making index_set_state_flags() transactional
Hi all,
For a couple of things I looked at lately, it would be really useful
to make index_state_set_flags() transactional and replace its use of
heap_inplace_update() by CatalogTupleUpdate():
- When dropping an index used in a replica identity, we could make the
reset of relreplident for the parent table consistent with the moment
the index is marked as invalid:
/messages/by-id/20200831062304.GA6555@paquier.xyz
- In order to make CREATE INDEX CONCURRENTLY work for partitioned
tables, we need to be able to switch indisvalid for all the index
partitions in one final transaction so as we have a consistent
partition tree at any state of the operation. Obviously,
heap_inplace_update() is a problem here.
The restriction we have in place now can be lifted thanks to 813fb03,
that removed SnapshotNow, and as far as I looked at the code paths
doing scans of pg_index, I don't see any reasons why we could not make
this stuff transactional. Perhaps that's just because nobody had use
cases where it would be useful, and I have two of them lately. Note
that 3c84046 was the original commit that introduced the use of
heap_inplace_update() and index_state_set_flags(), but we don't have
SnapshotNow anymore. Note as well that we already make the index
swapping transactional in REINDEX CONCURRENTLY for the pg_index
entries.
I got the feeling that this is an issue different than the other
threads where this was discussed, which is why I am beginning a new
thread. Any thoughts? Attached is a proposal of patch.
Thanks,
--
Michael
Attachments:
reindex-transactional-v1.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1d662e9af4..9b956071f6 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3297,18 +3297,10 @@ validate_index_callback(ItemPointer itemptr, void *opaque)
* 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() does send a cache inval 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)
@@ -3317,9 +3309,6 @@ 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);
-
/* Open pg_index and fetch a writable copy of the index's tuple */
pg_index = table_open(IndexRelationId, RowExclusiveLock);
@@ -3383,8 +3372,8 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
break;
}
- /* ... and write it back in-place */
- heap_inplace_update(pg_index, indexTuple);
+ /* ... and update it */
+ CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
table_close(pg_index, RowExclusiveLock);
}
On 03.09.2020 11:04, Michael Paquier wrote:
Hi all,
For a couple of things I looked at lately, it would be really useful
to make index_state_set_flags() transactional and replace its use of
heap_inplace_update() by CatalogTupleUpdate():
- When dropping an index used in a replica identity, we could make the
reset of relreplident for the parent table consistent with the moment
the index is marked as invalid:
/messages/by-id/20200831062304.GA6555@paquier.xyz
- In order to make CREATE INDEX CONCURRENTLY work for partitioned
tables, we need to be able to switch indisvalid for all the index
partitions in one final transaction so as we have a consistent
partition tree at any state of the operation. Obviously,
heap_inplace_update() is a problem here.The restriction we have in place now can be lifted thanks to 813fb03,
that removed SnapshotNow, and as far as I looked at the code paths
doing scans of pg_index, I don't see any reasons why we could not make
this stuff transactional. Perhaps that's just because nobody had use
cases where it would be useful, and I have two of them lately. Note
that 3c84046 was the original commit that introduced the use of
heap_inplace_update() and index_state_set_flags(), but we don't have
SnapshotNow anymore. Note as well that we already make the index
swapping transactional in REINDEX CONCURRENTLY for the pg_index
entries.I got the feeling that this is an issue different than the other
threads where this was discussed, which is why I am beginning a new
thread. Any thoughts? Attached is a proposal of patch.Thanks,
--
Michael
As this patch is needed to continue the work started in
"reindex/cic/cluster of partitioned tables" thread, I took a look on it.
I agree that transactional update in index_set_state_flags() is good for
both cases, that you mentioned and don't see any restrictions. It seems
that not doing this before was just a loose end, as the comment from
813fb03 patch clearly stated, that it is safe to make this update
transactional.
The patch itself looks clear and committable.
--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Fri, Sep 11, 2020 at 06:42:09PM +0300, Anastasia Lubennikova wrote:
I agree that transactional update in index_set_state_flags() is good for
both cases, that you mentioned and don't see any restrictions. It seems that
not doing this before was just a loose end, as the comment from 813fb03
patch clearly stated, that it is safe to make this update transactional.The patch itself looks clear and committable.
Thanks for double-checking, Anastasia. Committed.
--
Michael