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
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 |