On Thu, Jul 23, 2015 at 3:01 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:
> Hello.
> I’ve changed the previous patch. `group` field in LWLock is removed, so the size of LWLock will not increase.
> Instead of the `group` field I've created new tranches for LWLocks from MainLWLocksArray. This allowed to remove a
loop
> from the previous version of the patch.
> Now the names for LWLocks that are not individual is obtained from corresponding tranches.
I think using tranches for this is good, but I'm not really keen on
using two bytes for this. I think using one byte is just fine. It's
OK to assume that anything up to a 4-byte word can be read atomically
if it's read using a read of the same width that was used to write it.
But it's not OK to assume that if somebody might do, say, a memcpy of
a structure containing that 4-byte word, as pgstat_read_current_status
does. That, I think, might get torn, because it's reading using
1-byte reads. You'll notice that pgstat_beshutdown_hook() uses the
changecount protocol even though it's only changing a 4-byte word.
There's no real need for two bytes here, so let's not do that. Just
use offsets intelligently to pack it into a single byte.
Also, the patch should not invent a new array similar but not quite
identical to LockTagTypeNames[].
This is goofy:
+ if (tranche_id > 0)
+ result->tranche = tranche_id;
+ else
+ result->tranche = LW_USERDEFINED;
If we're going to make everybody pass a tranche ID when they call
LWLockAssign(), then they should have to do that always, not sometimes
pass a tranche ID and otherwise pass 0, which is actually a valid
tranche ID but to which you've given a weird special meaning here.
I'd also name the constants differently, like LWLOCK_TRANCHE_XXX or
something. LW_ is a somewhat ambiguous prefix.
The idea of tranches is really that each tranche is an array of items
each of which contains 1 or more lwlocks. Here you are intermingling
different tranches. I guess that won't really break anything but it
seems ugly.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company