Re: Confusing variable naming in LWLockRelease

From: Jacob Brazeal <jacob(dot)brazeal(at)gmail(dot)com>
To: Jeff Davis <pgsql(at)j-davis(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Confusing variable naming in LWLockRelease
Date: 2025-02-09 20:17:27
Message-ID: CA+COZaAF_HRPD0mc4ywseAYLphk77QUMvfmSUxzke+RSn2DBgg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> I believe it refers to the state of the lock prior to lock acquisition;
not prior to subtraction.

That definitely makes sense as a way to read this variable in context, but
after reviewing other usages of old_state in lwlock.c, I tend to think that
this is an outlier usage and maybe the naming was just copy-pasted from one
of the other compare-and-swaps, which tend to use "old state" to mean "the
state just before we mutated it.". Here are three other usages that make me
think this:

First, in LWLockWaitListLock, we have:

old_state = pg_atomic_fetch_or_u32(&lock->state, LW_FLAG_LOCKED);

Here old_state contains the lock state *before* the compare and swap, the
opposite of the case we considered above, but it does also work out to "the
state prior to lock acquisition".

Next, in LWLockWaitListUnlock, we have:

old_state = pg_atomic_fetch_and_u32(&lock->state, ~LW_FLAG_LOCKED);
Assert(old_state & LW_FLAG_LOCKED);

... so in this case "old_state" means "the locked state just prior to the
unlock."

Third, In LWLockWaitListUnlock, we have:

old_state = pg_atomic_read_u32(&lock->state);

which also refers to the locked state, prior to maybe being unlocked.

I want to say that in these 3 other examples, we are generally using
"old_state" to refer to "the state immediately prior to locally mutating
it," which can sometimes mean a locked and sometimes unlocked state. To me,
that's why the usage in LWLockRelease feels a little confusing, since it is
referring to "the state after we mutate it, but corresponding to an older
semantic state."

> But the name "newstate" wouldn't be great, either.
> I don't have a great name in mind, so perhaps a comment instead?

Hmm, maybe "unlocked_state" would be a better name in LWLockRelease, when
considered across these examples? Alternatively a comment like "Calculate
the "old" state, that is, prior to lock acquisition."

Regards,
Jacob

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Jacob Brazeal 2025-02-09 20:41:58 Re: MAX_BACKENDS size (comment accuracy)
Previous Message Jelte Fennema-Nio 2025-02-09 20:06:02 Re: RFC: Allow EXPLAIN to Output Page Fault Information