Hi,
On 2022-07-05 11:20:54 +1200, Thomas Munro wrote:
> On Tue, Jul 5, 2022 at 8:54 AM Andres Freund <andres@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