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: https://www.postgresql.org/message-id/20200831062304.ga6...@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
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); }
signature.asc
Description: PGP signature
