Re: [PATCH] Refactoring of LWLock tranches

On Mon, Feb 1, 2016 at 12:27 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Fixed.

This patch doesn't build:

./xfunc.sgml:                int                 lwlock_count = 0;
Tabs appear in SGML/XML files

The #define NUM_LWLOCKS 1 just seems totally unnecessary, as does int
lwlock_count = 0.  You're only assigning one lock!  I'd just do
RequestAddinLWLockTranche("pg_stat_statements locks", 1); pgss->lock =
GetLWLockAddinTranche("pg_stat_statements locks")->lock; and call it
good.

I think we shouldn't foreclose the idea of core users of this facility
by using names like NumLWLocksByLoadableModules().  Why can't an
in-core client use this API?  I think instead of calling these "addin
tranches" we should call them "named tranches"; thus public APIs
RequestNamedLWLockTranche()
and GetNamedLWLockTranche(), and private variables
NamedLWLockTrancheRequests, NamedLWLockTrancheRequestsAllocated, etc.
In fact,

I do not see an obvious reason why the two looks in CreateLWLocks()
that end with "} while (++i < LWLockTrancheRequestsCount);" could not
be merged, and I believe that would be cleaner than what you've got
now.  Similarly, the two loops in GetLWLockAddinTranche() could also
be merged.  Just keep a running total and return it when you find a
match.

I think it would be a good idea to merge LWLockAddInTrancheShmemSize
into LWLockShmemSize.  I don't see why that function can't compute a
grand total and return it.

Overall, I think this is on the right track, but it still needs some
work to make it cleaner.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company