Hi,
On 2022-07-04 14:55:43 +1200, Thomas Munro wrote:
> Right, as seen in the build farm at [1]. Also reproducible with something
> like:
>
> @@ -269,6 +269,14 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size
> request_size,
> return false;
> }
>
> + /* XXX random fault injection */
> + if (op == DSM_OP_ATTACH && random() < RAND_MAX / 8)
> + {
> + close(fd);
> + elog(ERROR, "chaos");
> + return false;
> + }
> +
>
> I must have thought that it was easy and practical to write no-throw
> straight-line code and be sure to reach dshash_release_lock(), but I
> concede that it was a bad idea: even dsa_get_address() can throw*, and
> you're often likely to need to call that while accessing dshash
> elements. For example, in lookup_rowtype_tupdesc_internal(), there is
> a sequence dshash_find(), ..., dsa_get_address(), ...,
> dshash_release_lock(), and I must have considered the range of code
> between find and release to be no-throw, but now I know that it is
> not.
Yea - I'd go as far as saying that it's almost never feasible.
> > 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().
> @@ -572,13 +552,8 @@ dshash_release_lock(dshash_table *hash_table, void
> *entry)
> size_t partition_index = PARTITION_FOR_HASH(item->hash);
>
> Assert(hash_table->control->magic == DSHASH_MAGIC);
> - Assert(hash_table->find_locked);
> - Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition_index),
> -
> hash_table->find_exclusively_locked
> - ? LW_EXCLUSIVE
> : LW_SHARED));
> + 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.
Greetings,
Andres Freund
