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

On 12/5/21, 5:54 PM, "Michael Paquier" <michael@paquier.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