pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Cope with catcache entries becoming stale during detoasting. 6+ messages / 1 participants [nested] [flat]
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ad98fb14226ae6456fbaed7990ee7591cbe5efd2 Modified Files -------------- src/backend/utils/cache/catcache.c | 164 ++++++++++++++++++++++++++++--------- 1 file changed, 124 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/7e2561e1a258ced4250c5d5d1b3b0221930dbf7e Modified Files -------------- src/backend/utils/cache/catcache.c | 164 ++++++++++++++++++++++++++++--------- 1 file changed, 124 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/2a46a0df479321d00d01e09270d794a800af6c25 Modified Files -------------- src/backend/utils/cache/catcache.c | 164 ++++++++++++++++++++++++++++--------- 1 file changed, 124 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ REL_14_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/db122d426a2dc6097c8a70b161769ea96de93bd1 Modified Files -------------- src/backend/utils/cache/catcache.c | 163 ++++++++++++++++++++++++++++--------- 1 file changed, 123 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ REL_13_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/98e03f95743624a4418d0d4f549aa1799d4bf28e Modified Files -------------- src/backend/utils/cache/catcache.c | 163 ++++++++++++++++++++++++++++--------- 1 file changed, 123 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
* pgsql: Cope with catcache entries becoming stale during detoasting. @ 2024-01-13 18:54 Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 6+ messages in thread From: Tom Lane @ 2024-01-13 18:54 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org Cope with catcache entries becoming stale during detoasting. We've long had a policy that any toasted fields in a catalog tuple should be pulled in-line before entering the tuple in a catalog cache. However, that requires access to the catalog's toast table, and we'll typically do AcceptInvalidationMessages while opening the toast table. So it's possible that the catalog tuple is outdated by the time we finish detoasting it. Since no cache entry exists yet, we can't mark the entry stale during AcceptInvalidationMessages, and instead we'll press forward and build an apparently-valid cache entry. The upshot is that we have a race condition whereby an out-of-date entry could be made in a backend's catalog cache, and persist there indefinitely causing indeterminate misbehavior. To fix, use the existing systable_recheck_tuple code to recheck whether the catalog tuple is still up-to-date after we finish detoasting it. If not, loop around and restart the process of searching the catalog and constructing cache entries from the top. The case is rare enough that this shouldn't create any meaningful performance penalty, even in the SearchCatCacheList case where we need to tear down and reconstruct the whole list. Indeed, the case is so rare that AFAICT it doesn't occur during our regression tests, and there doesn't seem to be any easy way to build a test that would exercise it reliably. To allow testing of the retry code paths, add logic (in USE_ASSERT_CHECKING builds only) that randomly pretends that the recheck failed about one time out of a thousand. This is enough to ensure that we'll pass through the retry paths during most regression test runs. By adding an extra level of looping, this commit creates a need to reindent most of SearchCatCacheMiss and SearchCatCacheList. I'll do that separately, to allow putting those changes in .git-blame-ignore-revs. Patch by me; thanks to Alexander Lakhin for having built a test case to prove the bug is real, and to Xiaoran Wang for review. Back-patch to all supported branches. Discussion: https://postgr.es/m/1393953.1698353013@sss.pgh.pa.us Discussion: https://postgr.es/m/CAGjhLkOoBEC9mLsnB42d3CO1vcMx71MLSEuigeABbQ8oRdA6gw@mail.gmail.com Branch ------ REL_12_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/3b4d85cf159c1d436bef251cb47f77671e1d99dc Modified Files -------------- src/backend/utils/cache/catcache.c | 163 ++++++++++++++++++++++++++++--------- 1 file changed, 123 insertions(+), 40 deletions(-) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-01-13 18:54 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us> 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us> 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us> 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us> 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us> 2024-01-13 18:54 pgsql: Cope with catcache entries becoming stale during detoasting. Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox