On Tue, Jul 5, 2022 at 8:54 AM Andres Freund <andres@anarazel.de> wrote:
> On 2022-07-04 14:55:43 +1200, Thomas Munro wrote:
> > > It's per-backend state at least and just used for assertions. We could remove
> > > it. Or stop checking it in places where it could be set wrongly: dshash_find()
> > > and dshash_detach() couldn't check anymore, but the rest of the assertions
> > > would still be valid afaics?
> >
> > 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. 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);
> > + Assert(LWLockHeldByMe(PARTITION_LOCK(hash_table, partition_index)));
> >
> > - hash_table->find_locked = false;
> > - hash_table->find_exclusively_locked = false;
> > LWLockRelease(PARTITION_LOCK(hash_table, partition_index));
> This LWLockHeldByMe() doesn't add much - the LWLockRelease() will error out if
> we don't hold the lock.
Duh. Removed.