Horton

Thread

Commits

  1. Make index_set_state_flags() transactional

  1. Making index_set_state_flags() transactional

    Michael Paquier <michael@paquier.xyz> — 2020-09-03T08:04:40Z

    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.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

  2. Re: Making index_set_state_flags() transactional

    Anastasia Lubennikova <a.lubennikova@postgrespro.ru> — 2020-09-11T15:42:09Z

    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:
> https://www.postgresql.org/message-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

  3. Re: Making index_set_state_flags() transactional

    Michael Paquier <michael@paquier.xyz> — 2020-09-14T04:58:56Z

    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