Thanks for diagnosing this!
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.
--
Heikki Linnakangas
Neon (https://neon.tech)
