On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote: > I carefully inspected all the code paths this patch touches, and I think > I've got all the details right, but I would be grateful if someone else > could take a look.
No objections from here with putting the snapshots pops and pushes outside the inner routines of reindex/drop concurrently, meaning that ReindexRelationConcurrently(), DefineIndex() and index_drop() are fine to do these operations. Looking at the patch, we could just add an assertion based on ActiveSnapshotSet() in index_set_state_flags(). Actually, thinking more... Could it be better to have some more sanity checks in the stack outside the toast code for catalogs with toast tables? For example, I could imagine adding a check in CatalogTupleUpdate() so as all catalog accessed that have a toast relation require an active snapshot. That would make checks more aggressive, because we would not need any toast data in a catalog to make sure that there is a snapshot set. This strikes me as something we could do better to improve the detection of failures like the one reported by Alexander when updating catalog tuples as this can be triggered each time we do a CatalogTupleUpdate() when dirtying a catalog tuple. The idea is then to have something before the HaveRegisteredOrActiveSnapshot() in the toast internals, for catalogs, and we would not require toast data to detect problems. -- Michael
signature.asc
Description: PGP signature
