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.munro@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)