[PATCH] Refactoring of LWLock tranches

Hi,

On 2015-09-05 12:48:12 +0300, Ildus Kurbangaliev wrote:

Another parts require a some discussion so I didn't touch them yet.

At this point I don't see any point in further review until these are
addressed.

The idea to create an individual tranches for individual LWLocks have
come from Heikki Linnakangas and I also think that tranche is a good place to keep
LWLock name.

I think it's rather ugly.

Base of these tranches points to MainLWLockArray

And that's just plain wrong. The base of a tranche ought to point to the
first lwlock in it.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sep 6, 2015, at 2:36 PM, andres@anarazel.de wrote:

On 2015-09-05 12:48:12 +0300, Ildus Kurbangaliev wrote:

Another parts require a some discussion so I didn't touch them yet.

At this point I don't see any point in further review until these are
addressed.

The idea to create an individual tranches for individual LWLocks have
come from Heikki Linnakangas and I also think that tranche is a good place to keep
LWLock name.

I think it's rather ugly.

Base of these tranches points to MainLWLockArray

And that's just plain wrong. The base of a tranche ought to point to the
first lwlock in it.

Ok, I've kept only one tranche for individual LWLocks

On Sep 2, 2015, at 1:43 AM, andres@anarazel.de wrote:

I don't really like the tranche model as in the patch right now. I'd
rather have in a way that we have one tranch for all the individual
lwlocks, where the tranche points to an array of names alongside the
tranche's name. And then for the others we just supply the tranche name,
but leave the name array empty, whereas a name can be generated.

Maybe I don't understand something here, but why add extra field to all tranches
if we need only one array (especially the array for individual LWLocks).

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com <http://www.postgrespro.com/&gt;
The Russian Postgres Company

On 2015-09-06 22:57:04 +0300, Ildus Kurbangaliev wrote:

Ok, I've kept only one tranche for individual LWLocks

But you've added the lock names as a statically sized array to all
tranches? Why not just a pointer to an array that's either NULL ('not
individualy named locks') or appropriately sized?

I don't really like the tranche model as in the patch right now. I'd
rather have in a way that we have one tranch for all the individual
lwlocks, where the tranche points to an array of names alongside the
tranche's name. And then for the others we just supply the tranche name,
but leave the name array empty, whereas a name can be generated.

Maybe I don't understand something here, but why add extra field to all tranches
if we need only one array (especially the array for individual LWLocks).

It's cheap to add an optional pointer field to an individual
tranche. It'll be far less cheap to extend the max number of
tranches. But it's also just ugly to to use a tranche per lock - they're
intended to describe 'runs' of locks.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, 6 Sep 2015 23:18:02 +0200
"andres@anarazel.de" <andres@anarazel.de> wrote:

On 2015-09-06 22:57:04 +0300, Ildus Kurbangaliev wrote:

Ok, I've kept only one tranche for individual LWLocks

But you've added the lock names as a statically sized array to all
tranches? Why not just a pointer to an array that's either NULL ('not
individualy named locks') or appropriately sized?

A tranche contains only the tranche name. Yes, it's statically sized,
because we need to know an exact space in shared memory for it. This
name is enough to describe all LWLocks in that tranche (except main
tranche), because they have one type (for example BufferMgrLocks).
In main tranche this field contains 'main' (like now) and
an additional array is used for determining a name for LWLock. If you
suggest keep a pointer to this array in main tranche (and NULL for
others) then I have no objections to do that.

So generally the code is similar to code that we have in Postgres now
except the tranches located in shared memory and created by backends.

I don't really like the tranche model as in the patch right now.
I'd rather have in a way that we have one tranch for all the
individual lwlocks, where the tranche points to an array of names
alongside the tranche's name. And then for the others we just
supply the tranche name, but leave the name array empty, whereas
a name can be generated.

Maybe I don't understand something here, but why add extra field to
all tranches if we need only one array (especially the array for
individual LWLocks).

It's cheap to add an optional pointer field to an individual
tranche. It'll be far less cheap to extend the max number of
tranches. But it's also just ugly to to use a tranche per lock -
they're intended to describe 'runs' of locks.

The current patch exactly does that - contains 'runs' of locks.

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
<http://www.postgrespro.com/&gt; The Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Added changes related to the latest master (for individual LWLocks
definitions)

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Sun, Sep 13, 2015 at 12:43 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Added changes related to the latest master (for individual LWLocks
definitions)

If I haven't said this clearly enough already, I'm not OK with
changing the tranche name from char * to a fixed-size character array.
Nor am I OK with limiting the maximum number of tranches to 64. I
worked hard to set this system up so that it did not have limits on
the number of tranches or the lengths of their names, and I don't see
any good reason to add those limitations now.

I would like to avoid adding an argument to every call to
SimpleLruInit(). It's already got two arguments that are basically
names; let's avoid introducing a third one. Instead, let's decide on
a naming convention that we ca use for both locks and shared memory
segments. We haven't worried about that much in the past because this
stuff was only exposed to developers, but that's changing now. So
let's come up with something that will be nice for users and adopt a
uniform convention. I don't think it should be NameOfSubsystemLocks
as you have it here. That's too easy to confuse with heavyweight
locks.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sep 13, 2015, at 11:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Sep 13, 2015 at 12:43 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Added changes related to the latest master (for individual LWLocks
definitions)

If I haven't said this clearly enough already, I'm not OK with
changing the tranche name from char * to a fixed-size character array.
Nor am I OK with limiting the maximum number of tranches to 64. I
worked hard to set this system up so that it did not have limits on
the number of tranches or the lengths of their names, and I don't see
any good reason to add those limitations now.

Yes, that is because I tried to go with current convention working with
shmem in Postgres (there are one function that returns the size and
others that initialize that memory). But I like your suggestion about
API functions, in that case number of tranches and locks will be known
during the initialization.

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com <http://www.postgrespro.com/&gt;
The Russian Postgres Company

On Sun, Sep 13, 2015 at 5:05 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, that is because I tried to go with current convention working with
shmem in Postgres (there are one function that returns the size and
others that initialize that memory). But I like your suggestion about
API functions, in that case number of tranches and locks will be known
during the initialization.

I also want to leave the door open to tranches that are registered
after initialization. At that point, it's too late to put a tranche
in shared memory, but you can still use DSM.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, 14 Sep 2015 06:32:22 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Sep 13, 2015 at 5:05 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, that is because I tried to go with current convention working
with shmem in Postgres (there are one function that returns the
size and others that initialize that memory). But I like your
suggestion about API functions, in that case number of tranches and
locks will be known during the initialization.

I also want to leave the door open to tranches that are registered
after initialization. At that point, it's too late to put a tranche
in shared memory, but you can still use DSM.

We can hold some extra space in LWLockTrancheArray, add some
function for unregistering a tranche, and reuse free items in
LWLockTrancheId later.

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 15, 2015 at 12:44 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

On Mon, 14 Sep 2015 06:32:22 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Sep 13, 2015 at 5:05 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, that is because I tried to go with current convention working
with shmem in Postgres (there are one function that returns the
size and others that initialize that memory). But I like your
suggestion about API functions, in that case number of tranches and
locks will be known during the initialization.

I also want to leave the door open to tranches that are registered
after initialization. At that point, it's too late to put a tranche
in shared memory, but you can still use DSM.

We can hold some extra space in LWLockTrancheArray, add some
function for unregistering a tranche, and reuse free items in
LWLockTrancheId later.

We could, but since that would be strictly more annoying and less
flexible than what we've already got, why would we?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-09-15 14:39:51 -0400, Robert Haas wrote:

We could, but since that would be strictly more annoying and less
flexible than what we've already got, why would we?

I don't find the current approach of having to define tranches in every
backend all that convenient. It also completely breaks down if you want
to display locks from tranches that are only visible within a subset of
the backends - not that unlikely given that shared_preload_libraries is
a PITA.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 15, 2015 at 2:44 PM, andres@anarazel.de <andres@anarazel.de> wrote:

On 2015-09-15 14:39:51 -0400, Robert Haas wrote:

We could, but since that would be strictly more annoying and less
flexible than what we've already got, why would we?

I don't find the current approach of having to define tranches in every
backend all that convenient.

Sure. That's why I proposed a way to define them across all backends
during postmaster startup. However, I'd like that way to be in
addition to what works now, not instead of it. That way, you can do
it the convenient way if it applies, and the inconvenient way is still
there if you really need it.

It also completely breaks down if you want
to display locks from tranches that are only visible within a subset of
the backends - not that unlikely given that shared_preload_libraries is
a PITA.

True. I don't really see that as a big deal. We can expect that most
people will register tranches at startup, and the names will be
available. If they don't, then the tranche name will have to be
listed as something like extension, or maybe extension-tranche-123.
I'm not prepared to revoke the ability to create tranches later for
the convenience of this mechanism; the reporting mechanism has to
adapt itself to what the code can support, not the other way around.

Actually, I think it might very well be a good idea to fold all
non-core tranches together under "extension" anyway for wait reporting
purposes. The reason is that we are debating whether to report the
wait event information as 1 byte or 4 bytes. The tranche ID by itself
is 4 bytes, and lwlocks are not the only kind of wait event we need to
be able to report. Unless we're prepared to make the wait-reporting
stuff use more than 4 bytes, and therefore require a locking protocol
(which I'm not keen about for reasons discussed upthread), we're not
going to be able to report the full range of values anyway.

My proposal is to have about 50 possible wait events for lwlocks: one
for each individual lwlock, one for each builtin tranche, and one for
everything else. That easily fits in one byte with plenty of room
left over for all of the other stuff I want to report. If we've got
four bytes, it's even simpler. If users of this facility can't
distinguish a wait on an lwlock created by extension A from a wait on
an lwlock created by extension B, I don't really care. That's going
to be a really, really rare case: how many extensions use lwlocks
anyway? How many of them use them enough to cause significant
contention? Yeah, it could happen, but not often.

Something pretty simple, dumb, and cheap here still figures to offer a
lot of benefit. In fact, probably *more* benefit than a more complex,
more general system.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, 15 Sep 2015 14:39:51 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Sep 15, 2015 at 12:44 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

On Mon, 14 Sep 2015 06:32:22 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Sep 13, 2015 at 5:05 PM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, that is because I tried to go with current convention
working with shmem in Postgres (there are one function that
returns the size and others that initialize that memory). But I
like your suggestion about API functions, in that case number of
tranches and locks will be known during the initialization.

I also want to leave the door open to tranches that are registered
after initialization. At that point, it's too late to put a
tranche in shared memory, but you can still use DSM.

We can hold some extra space in LWLockTrancheArray, add some
function for unregistering a tranche, and reuse free items in
LWLockTrancheId later.

We could, but since that would be strictly more annoying and less
flexible than what we've already got, why would we?

Yes, probably.
I'm going to change API calls as you suggested earlier.
How you do think the tranches registration after initialization should
look like?

----
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Sep 22, 2015 at 5:16 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, probably.
I'm going to change API calls as you suggested earlier.
How you do think the tranches registration after initialization should
look like?

I don't see any need to change anything there. The idea there is that
an extension allocates a tranche ID and are responsible for making
sure that every backend that uses that tranche finds out about the ID
that was chosen and registers a matching tranche definition. How to
do that is the extension's problem. Maybe eventually we'll provide
some tools to make that easier, but that's separate from the work
we're trying to do here.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas wrote:

On Tue, Sep 22, 2015 at 5:16 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, probably.
I'm going to change API calls as you suggested earlier.
How you do think the tranches registration after initialization should
look like?

I don't see any need to change anything there. The idea there is that
an extension allocates a tranche ID and are responsible for making
sure that every backend that uses that tranche finds out about the ID
that was chosen and registers a matching tranche definition. How to
do that is the extension's problem. Maybe eventually we'll provide
some tools to make that easier, but that's separate from the work
we're trying to do here.

FWIW I had assumed, when you created the tranche stuff, that SLRU users
would all allocate their lwlocks from a tranche provided by slru.c
itself, and the locks would be stored in the slru Ctl struct. Does that
not work for some reason?

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 23, 2015 at 11:22 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Robert Haas wrote:

On Tue, Sep 22, 2015 at 5:16 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, probably.
I'm going to change API calls as you suggested earlier.
How you do think the tranches registration after initialization should
look like?

I don't see any need to change anything there. The idea there is that
an extension allocates a tranche ID and are responsible for making
sure that every backend that uses that tranche finds out about the ID
that was chosen and registers a matching tranche definition. How to
do that is the extension's problem. Maybe eventually we'll provide
some tools to make that easier, but that's separate from the work
we're trying to do here.

FWIW I had assumed, when you created the tranche stuff, that SLRU users
would all allocate their lwlocks from a tranche provided by slru.c
itself, and the locks would be stored in the slru Ctl struct. Does that
not work for some reason?

I think that should work and that it's a good idea. I think it's just
a case of nobody having done the work.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, 23 Sep 2015 11:46:00 -0400
Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Sep 23, 2015 at 11:22 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Robert Haas wrote:

On Tue, Sep 22, 2015 at 5:16 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

Yes, probably.
I'm going to change API calls as you suggested earlier.
How you do think the tranches registration after initialization
should look like?

I don't see any need to change anything there. The idea there is
that an extension allocates a tranche ID and are responsible for
making sure that every backend that uses that tranche finds out
about the ID that was chosen and registers a matching tranche
definition. How to do that is the extension's problem. Maybe
eventually we'll provide some tools to make that easier, but
that's separate from the work we're trying to do here.

FWIW I had assumed, when you created the tranche stuff, that SLRU
users would all allocate their lwlocks from a tranche provided by
slru.c itself, and the locks would be stored in the slru Ctl
struct. Does that not work for some reason?

I think that should work and that it's a good idea. I think it's just
a case of nobody having done the work.

There is a patch that splits SLRU LWLocks to separate tranches and
moves them to SLRU Ctl. It does some work from the main patch from
this thread, but can be commited separately. It also simplifies
lwlock.c.

--
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Fri, Nov 6, 2015 at 6:27 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

There is a patch that splits SLRU LWLocks to separate tranches and
moves them to SLRU Ctl. It does some work from the main patch from
this thread, but can be commited separately. It also simplifies
lwlock.c.

Thanks. I like the direction this is going.

-               char       *ptr;
-               Size            offset;
-               int                     slotno;
+               char            *ptr;
+               Size             offset;
+               int              slotno;
+               int              tranche_id;
+               LWLockPadded    *locks;

Please don't introduce this kind of churn. pgindent will undo it.

This isn't going to work for EXEC_BACKEND builds, I think. It seems
to rely on the LWLockRegisterTranche() performed !IsUnderPostmaster
being inherited by subsequent children, which won't work under
EXEC_BACKEND. Instead, store the tranche ID in SlruSharedData. Move
the LWLockRegisterTranche call out from the (!IsUnderPostmaster) case
and call it based on the tranche ID from SlruSharedData.

I would just drop the add_postfix stuff. I think it's fine if the
names of the shared memory checks are just "CLOG" etc. rather than
"CLOG Slru Ctl", and similarly I think the locks can be registered
without the "Locks" suffix. It'll be clear from context that they are
locks. I suggest also that we just change all of these names to be
lower case, though I realize that's a debatable and cosmetic point.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11/06/2015 08:53 PM, Robert Haas wrote:

On Fri, Nov 6, 2015 at 6:27 AM, Ildus Kurbangaliev
<i.kurbangaliev@postgrespro.ru> wrote:

There is a patch that splits SLRU LWLocks to separate tranches and
moves them to SLRU Ctl. It does some work from the main patch from
this thread, but can be commited separately. It also simplifies
lwlock.c.

Thanks. I like the direction this is going.

-               char       *ptr;
-               Size            offset;
-               int                     slotno;
+               char            *ptr;
+               Size             offset;
+               int              slotno;
+               int              tranche_id;
+               LWLockPadded    *locks;

Please don't introduce this kind of churn. pgindent will undo it.

This isn't going to work for EXEC_BACKEND builds, I think. It seems
to rely on the LWLockRegisterTranche() performed !IsUnderPostmaster
being inherited by subsequent children, which won't work under
EXEC_BACKEND. Instead, store the tranche ID in SlruSharedData. Move
the LWLockRegisterTranche call out from the (!IsUnderPostmaster) case
and call it based on the tranche ID from SlruSharedData.

I would just drop the add_postfix stuff. I think it's fine if the
names of the shared memory checks are just "CLOG" etc. rather than
"CLOG Slru Ctl", and similarly I think the locks can be registered
without the "Locks" suffix. It'll be clear from context that they are
locks. I suggest also that we just change all of these names to be
lower case, though I realize that's a debatable and cosmetic point.

Thanks for the review. I've attached a new version of SLRU patch. I've
removed
add_postfix and fixed EXEC_BACKEND case.

--
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company