Re: BUG #17268: Possible corruption in toast index after reindex index concurrently

On 12/5/21, 5:54 PM, "Michael Paquier" <michael(at)paquier(dot)xyz> wrote:
> I have been working on this one again for the last couple of days, and
> I would still go with the simple solution, releasing the row-level
> toast locks only at the end of the transaction when saving and
> deleting a toast value. While testing, I have noticed that the
> deletion part is also important, as a REINDEX CONCURRENTLY run in
> parallel of a transaction removing a toast value would go through
> without that, rather than waiting for the transaction doing the
> deletion to commit first. I have expanded the tests with everything I
> could think about, so I'd like to commit the attached. The test is
> fancy with its use of allow_system_table_mods to make the toast
> relation names reliable, but it has been really useful.

I confirmed that the new tests reliably produce corruption and that
the suggested fix resolves it. I also lean towards the simple
solution, but I do wonder if it creates any interesting side effects.
For example, could holding the locks longer impact performance? (Of
course, performance is probably not a great reason to avoid a
sustainable solution.)

- toast_close_indexes(toastidxs, num_indexes, RowExclusiveLock);
- table_close(toastrel, RowExclusiveLock);
+ toast_close_indexes(toastidxs, num_indexes, NoLock);
+ table_close(toastrel, NoLock);

I think it would be good to expand the comments above these changes to
explain why we are keeping the lock. That might help avoid similar
problems in the future.

Nathan