Re: [PATCH] Refactoring of LWLock tranches

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Ildus Kurbangaliev <i(dot)kurbangaliev(at)postgrespro(dot)ru>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
Subject: Re: [PATCH] Refactoring of LWLock tranches
Date: 2016-02-04 12:00:35
Message-ID: CAA4eK1+Dr_9y+vXi0koFNfhaDYeR8k8zxj1b5d_RZOv8K2AVVQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Tue, Feb 2, 2016 at 7:19 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Feb 1, 2016 at 12:27 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > Fixed.
>
> This patch doesn't build:
>
> ./xfunc.sgml: int lwlock_count = 0;
> Tabs appear in SGML/XML files
>

Changed the documentation and now I am not getting build failure.

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

Changed as per suggestion.

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

Changed as per suggestion.

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

Agreed with both the points and changed as per suggestion.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Attachment Content-Type Size
separate_tranche_extensions_v4.patch application/octet-stream 15.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2016-02-04 12:06:10 Re: Raising the checkpoint_timeout limit
Previous Message Michael Paquier 2016-02-04 11:34:00 Re: silent data loss with ext4 / all current versions