On 10/03/2024 11:20, Heikki Linnakangas wrote:
On 10/03/2024 08:23, Thomas Munro wrote:
On Sun, Mar 10, 2024 at 6:48 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
I won't be surprised if the answer is: if you're holding a reference,
you have to get a pin (referring to bulk_write.c).
Ahhh, on second thoughts, I take that back, I think the original
theory still actually works just fine. It's just that somewhere in
our refactoring of that commit, when we were vacillating between
different semantics for 'destroy' and 'release', I think we made a
mistake: in RelationCacheInvalidate() I think we should now call
smgrreleaseall(), not smgrdestroyall().
Yes, I ran the reproducer with 'rr', and came to the same conclusion.
That smgrdestroyall() call closes destroys the SmgrRelation, breaking
the new assumption that an unpinned SmgrRelation is valid until end of
transaction.
That satisfies the
requirements for sinval queue overflow: we close md.c segments (and
most importantly virtual file descriptors), so our lack of sinval
records can't hurt us, we'll reopen all files as required. That's
what CLOBBER_CACHE_ALWAYS is effectively testing (and more). But the
smgr pointer remains valid, and retains only its "identity", eg hash
table key, and that's also fine. It won't be destroyed until after
the end of the transaction. Which was the point, and it allows things
like bulk_write.c (and streaming_read.c) to hold an smgr reference.
Right
+1.
I wonder if we can now simplify RelationCacheInvalidate() further. In
the loop above that smgrdestroyall():
while ((idhentry = (RelIdCacheEnt *) hash_seq_search(&status)) != NULL)
{
relation = idhentry->reldesc;
/* Must close all smgr references to avoid leaving dangling
ptrs */
RelationCloseSmgr(relation);
/*
* Ignore new relations; no other backend will manipulate them
before
* we commit. Likewise, before replacing a relation's
relfilelocator,
* we shall have acquired AccessExclusiveLock and drained any
* applicable pending invalidations.
*/
if (relation->rd_createSubid != InvalidSubTransactionId ||
relation->rd_firstRelfilelocatorSubid !=
InvalidSubTransactionId)
continue;
I don't think we need to call RelationCloseSmgr() for relations created
in the same transaction anymore.
Barring objections, I'll commit the attached.
Hmm, I'm not sure if we need even smgrreleaseall() here anymore. It's
not required for correctness AFAICS. We don't do it in single-rel
invalidation in RelationCacheInvalidateEntry() either.
--
Heikki Linnakangas
Neon (https://neon.tech)
From be03591a4c69224de5b96335aa4a29b3462e2bc9 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Sun, 10 Mar 2024 22:18:59 +0200
Subject: [PATCH 1/1] Don't destroy SMgrRelations at relcache invalidation
With commit 21d9c3ee4e, SMgrRelations remain valid until end of
transaction (or longer if they're "pinned"). Relcache invalidation can
happen in the middle of a transaction, so we must not destroy them at
relcache invalidation anymore.
This was revealed by failures in the 'constraints' test in buildfarm
animals using -DCLOBBER_CACHE_ALWAYS. That started failing with commit
8af2565248, which was the first commit that started to rely on an
SMgrRelation living until end of transaction.
Diagnosed-by: Tomas Vondra, Thomas Munro
Discussion: https://www.postgresql.org/message-id/CA%2BhUKGK%2B5DOmLaBp3Z7C4S-Yv6yoROvr1UncjH2S1ZbPT8D%2BZg%40mail.gmail.com
---
src/backend/utils/cache/relcache.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 7ad6a35a50e..2cd19d603fb 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2985,9 +2985,6 @@ RelationCacheInvalidate(bool debug_discard)
{
relation = idhentry->reldesc;
- /* Must close all smgr references to avoid leaving dangling ptrs */
- RelationCloseSmgr(relation);
-
/*
* Ignore new relations; no other backend will manipulate them before
* we commit. Likewise, before replacing a relation's relfilelocator,
@@ -3039,11 +3036,10 @@ RelationCacheInvalidate(bool debug_discard)
}
/*
- * Now zap any remaining smgr cache entries. This must happen before we
- * start to rebuild entries, since that may involve catalog fetches which
- * will re-open catalog files.
+ * We cannot destroy the SMgrRelations as there might still be references
+ * to them, but close the underlying file descriptors.
*/
- smgrdestroyall();
+ smgrreleaseall();
/* Phase 2: rebuild the items found to need rebuild in phase 1 */
foreach(l, rebuildFirstList)
--
2.39.2
