Hi, On 2022-07-05 11:20:54 +1200, Thomas Munro wrote: > On Tue, Jul 5, 2022 at 8:54 AM Andres Freund <and...@anarazel.de> wrote: > > > Yeah, it's all for assertions... let's just remove it. Those > > > assertions were useful to me at some stage in development but won't > > > hold as well as I thought, at least without widespread PG_FINALLY(), > > > which wouldn't be nice. > > > > Hm. I'd be inclined to at least add a few more > > Assert(!LWLockHeldByMe[InMode]()) style assertions. E.g. to > > dshash_find_or_insert(). > > Yeah, I was wondering about that, but it needs to check the whole 128 > element lock array.
I think it'd be ok to just check the current partition - yes, it'd not catch cases where we're still holding a lock on another partition, but that's imo not too bad? > Hmm, yeah that seems OK for assertion builds. > Since there were 6 places with I-hold-no-lock assertions, I shoved the > loop into a function so I could do: > > - Assert(!status->hash_table->find_locked); > + assert_no_lock_held_by_me(hash_table); I am a *bit* wary about the costs of that, even in assert builds - each of the partition checks in the loop will in turn need to iterate through held_lwlocks. But I guess we can also just later weaken them if it turns out to be a problem. Greetings, Andres Freund
