pgsql: Code cleanup in the wake of recent LWLock refactoring.

Code cleanup in the wake of recent LWLock refactoring.

As of commit c1772ad9225641c921545b35c84ee478c326b95e, there's no
longer any way of requesting additional LWLocks in the main tranche,
so we don't need NumLWLocks() or LWLockAssign() any more. Also,
some of the allocation counters that we had previously aren't needed
any more either.

Amit Kapila

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/79a7ff0fe56ac9d782b0734ebb0e7a5299015e58

Modified Files
--------------
src/backend/storage/lmgr/lwlock.c | 89 ++++++++-------------------------------
src/include/storage/lwlock.h | 2 -
2 files changed, 17 insertions(+), 74 deletions(-)

On 10/02/16 17:12, Robert Haas wrote:
> Code cleanup in the wake of recent LWLock refactoring.
>
> As of commit c1772ad9225641c921545b35c84ee478c326b95e, there's no
> longer any way of requesting additional LWLocks in the main tranche,
> so we don't need NumLWLocks() or LWLockAssign() any more. Also,
> some of the allocation counters that we had previously aren't needed
> any more either.

(Sorry if this was discussed already, I haven't been paying attention)

LWLockAssign() is used by extensions. Are we OK with just breaking them,
requiring them to change LWLockAssign() with the new mechanism, with
#ifdefs to support multiple server versions? Seems like it shouldn't be
too hard to keep LWLockAssign() around for the benefit of extensions, so
it seems a bit inconsiderate to remove it.

- Heikki

On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 10/02/16 17:12, Robert Haas wrote:
>> Code cleanup in the wake of recent LWLock refactoring.
>>
>> As of commit c1772ad9225641c921545b35c84ee478c326b95e, there's no
>> longer any way of requesting additional LWLocks in the main tranche,
>> so we don't need NumLWLocks() or LWLockAssign() any more. Also,
>> some of the allocation counters that we had previously aren't needed
>> any more either.
>
> (Sorry if this was discussed already, I haven't been paying attention)
>
> LWLockAssign() is used by extensions. Are we OK with just breaking them,
> requiring them to change LWLockAssign() with the new mechanism, with #ifdefs
> to support multiple server versions? Seems like it shouldn't be too hard to
> keep LWLockAssign() around for the benefit of extensions, so it seems a bit
> inconsiderate to remove it.

It was discussed on the "Refactoring of LWLock tranches" thread,
though there wasn't an overwhelming consensus or anything. I don't
think the burden on extension authors is very much, because you just
have to do this:

--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -404,7 +404,7 @@ _PG_init(void)
* resources in pgss_shmem_startup().
*/
RequestAddinShmemSpace(pgss_memsize());
- RequestAddinLWLocks(1);
+ RequestNamedLWLockTranche("pg_stat_statements", 1);

/*
* Install hooks.
@@ -480,7 +480,7 @@ pgss_shmem_startup(void)
if (!found)
{
/* First time through ... */
- pgss->lock = LWLockAssign();
+ pgss->lock =
&(GetNamedLWLockTranche("pg_stat_statements"))->lock;
pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
pgss->mean_query_len = ASSUMED_LENGTH_INIT;
SpinLockInit(&pgss->mutex);

We've gone through and done this to all of the EnterpriseDB
proprietary extensions over the last couple of days.

If there's a strong feeling that we should keep the old APIs around,
we can do that, but I think that (1) if we don't remove them now, we
probably never will and (2) they are vile APIs. Allocating the number
of add-in lwlocks that are requested or a minimum of 3 is just awful.
If somebody allocates a different number than they request it
sometimes works, except when combined with some other extension, when
it maybe doesn't work. This way, you ask for an LWLock under a given
name and then get it under that name, so if an extension does it
wrong, it is that extension that breaks rather than some other one. I
think that's enough benefit to justify requiring a small code change
on the part of extension authors that use LWLocks, but that's
obviously biased by my experience maintaining EDB's extensions, and
other people may well feel differently. But to me, the update that's
required here is no worse than what
5043193b78919a1bd144563aadc2f7f726549913 required, and I didn't hear
any complaints about that. You just go through and do to your code
what got done to the core contrib modules, and you're done.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> (Sorry if this was discussed already, I haven't been paying attention)
>>
>> LWLockAssign() is used by extensions. Are we OK with just breaking them,
>> requiring them to change LWLockAssign() with the new mechanism, with #ifdefs
>> to support multiple server versions? Seems like it shouldn't be too hard to
>> keep LWLockAssign() around for the benefit of extensions, so it seems a bit
>> inconsiderate to remove it.

> If there's a strong feeling that we should keep the old APIs around,
> we can do that, but I think that (1) if we don't remove them now, we
> probably never will and (2) they are vile APIs. Allocating the number
> of add-in lwlocks that are requested or a minimum of 3 is just awful.
> If somebody allocates a different number than they request it
> sometimes works, except when combined with some other extension, when
> it maybe doesn't work. This way, you ask for an LWLock under a given
> name and then get it under that name, so if an extension does it
> wrong, it is that extension that breaks rather than some other one. I
> think that's enough benefit to justify requiring a small code change
> on the part of extension authors that use LWLocks, but that's
> obviously biased by my experience maintaining EDB's extensions, and
> other people may well feel differently.

FWIW, I wasn't paying attention either, but I'm convinced by Robert's
argument. Avoiding coupling between extensions is worth an API break.

regards, tom lane

On 11 February 2016 at 00:21, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>
> If there's a strong feeling that we should keep the old APIs around,
> we can do that, but I think that (1) if we don't remove them now, we
> probably never will and (2) they are vile APIs.

Yep.

Delete 'em.

Things regularly change between releases in ways that're visible to
extensions, though not necessarily as obviously part of the extension API.
The most obvious being at least one change to ProcessUtility_hook and
probably other hook function signature changes over time.

It's a pain to have to #if around the differences, but better to export
that to extensions than *never* be able to retire cruft from core. Lets not
be Java, still stuck with Java 1.0 APIs everyone knows are unspeakably
awful.

Delete the APIs, relnote it with the required changes and be done with it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2016-02-10 17:21 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
> wrote:
> > On 10/02/16 17:12, Robert Haas wrote:
> >> Code cleanup in the wake of recent LWLock refactoring.
> >>
> >> As of commit c1772ad9225641c921545b35c84ee478c326b95e, there's no
> >> longer any way of requesting additional LWLocks in the main tranche,
> >> so we don't need NumLWLocks() or LWLockAssign() any more. Also,
> >> some of the allocation counters that we had previously aren't needed
> >> any more either.
> >
> > (Sorry if this was discussed already, I haven't been paying attention)
> >
> > LWLockAssign() is used by extensions. Are we OK with just breaking them,
> > requiring them to change LWLockAssign() with the new mechanism, with
> #ifdefs
> > to support multiple server versions? Seems like it shouldn't be too hard
> to
> > keep LWLockAssign() around for the benefit of extensions, so it seems a
> bit
> > inconsiderate to remove it.
>
> It was discussed on the "Refactoring of LWLock tranches" thread,
> though there wasn't an overwhelming consensus or anything. I don't
> think the burden on extension authors is very much, because you just
> have to do this:
>
> --- a/contrib/pg_stat_statements/pg_stat_statements.c
> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> @@ -404,7 +404,7 @@ _PG_init(void)
> * resources in pgss_shmem_startup().
> */
> RequestAddinShmemSpace(pgss_memsize());
> - RequestAddinLWLocks(1);
> + RequestNamedLWLockTranche("pg_stat_statements", 1);
>
> /*
> * Install hooks.
> @@ -480,7 +480,7 @@ pgss_shmem_startup(void)
> if (!found)
> {
> /* First time through ... */
> - pgss->lock = LWLockAssign();
> + pgss->lock =
> &(GetNamedLWLockTranche("pg_stat_statements"))->lock;
> pgss->cur_median_usage = ASSUMED_MEDIAN_INIT;
> pgss->mean_query_len = ASSUMED_LENGTH_INIT;
> SpinLockInit(&pgss->mutex);
>
> We've gone through and done this to all of the EnterpriseDB
> proprietary extensions over the last couple of days.
>
> If there's a strong feeling that we should keep the old APIs around,
> we can do that, but I think that (1) if we don't remove them now, we
> probably never will and (2) they are vile APIs. Allocating the number
> of add-in lwlocks that are requested or a minimum of 3 is just awful.
> If somebody allocates a different number than they request it
> sometimes works, except when combined with some other extension, when
> it maybe doesn't work. This way, you ask for an LWLock under a given
> name and then get it under that name, so if an extension does it
> wrong, it is that extension that breaks rather than some other one. I
> think that's enough benefit to justify requiring a small code change
> on the part of extension authors that use LWLocks, but that's
> obviously biased by my experience maintaining EDB's extensions, and
> other people may well feel differently. But to me, the update that's
> required here is no worse than what
> 5043193b78919a1bd144563aadc2f7f726549913 required, and I didn't hear
> any complaints about that. You just go through and do to your code
> what got done to the core contrib modules, and you're done.
>

There will be necessary more changes than this. Orafce has some parts based
on lw locks. I am able to compile it without any issue. But the lock
mechanism is broken now - so impact on extensions will be higher. Have to
do some research.

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

There will be necessary more changes than this. Orafce has some parts based
> on lw locks. I am able to compile it without any issue. But the lock
> mechanism is broken now - so impact on extensions will be higher. Have to
> do some research.
>

if somebody would to use it for testing

https://github.com/orafce/orafce
https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79

With last commit I am able to compile orafce without warnings, but
installcheck is broken. It can be bug in orafce, but this code worked last
7 years.

Pavel

> Regards
>
> Pavel
>
>
>
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>
>

On Fri, Feb 12, 2016 at 2:03 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
wrote:

>
>
> There will be necessary more changes than this. Orafce has some parts
>> based on lw locks. I am able to compile it without any issue. But the lock
>> mechanism is broken now - so impact on extensions will be higher. Have to
>> do some research.
>>
>
> if somebody would to use it for testing
>
> https://github.com/orafce/orafce
>
> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
>
> With last commit I am able to compile orafce without warnings, but
> installcheck is broken. It can be bug in orafce, but this code worked last
> 7 years.
>
>
One question regarding your latest commit in orafce:

- RequestAddinShmemSpace(SHMEMMSGSZ);
+#if PG_VERSION_NUM >= 90600
+
+ RequestNamedLWLockTranche("orafce", 1);
+
+#else
+
RequestAddinLWLocks(1);
+#endif
+
+ RequestAddinShmemSpace(SHMEMMSGSZ);
+

It seems you have moved request for shared memory
(RequestAddinShmemSpace()) after requesting locks, any reason
for same?

You don't need to change the request for shared memory.

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

2016-02-12 10:37 GMT+01:00 Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>:

> On Fri, Feb 12, 2016 at 2:03 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>>
>>
>> There will be necessary more changes than this. Orafce has some parts
>>> based on lw locks. I am able to compile it without any issue. But the lock
>>> mechanism is broken now - so impact on extensions will be higher. Have to
>>> do some research.
>>>
>>
>> if somebody would to use it for testing
>>
>> https://github.com/orafce/orafce
>>
>> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
>>
>> With last commit I am able to compile orafce without warnings, but
>> installcheck is broken. It can be bug in orafce, but this code worked last
>> 7 years.
>>
>>
> One question regarding your latest commit in orafce:
>
> - RequestAddinShmemSpace(SHMEMMSGSZ);
> +#if PG_VERSION_NUM >= 90600
> +
> + RequestNamedLWLockTranche("orafce", 1);
> +
> +#else
> +
> RequestAddinLWLocks(1);
> +#endif
> +
> + RequestAddinShmemSpace(SHMEMMSGSZ);
> +
>
>

> It seems you have moved request for shared memory
> (RequestAddinShmemSpace()) after requesting locks, any reason
> for same?
>

no, it is only moved after lock request

Pavel

>
> You don't need to change the request for shared memory.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>

On Fri, Feb 12, 2016 at 3:33 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> There will be necessary more changes than this. Orafce has some parts
>> based on lw locks. I am able to compile it without any issue. But the lock
>> mechanism is broken now - so impact on extensions will be higher. Have to do
>> some research.
>
> if somebody would to use it for testing
>
> https://github.com/orafce/orafce
> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
>
> With last commit I am able to compile orafce without warnings, but
> installcheck is broken. It can be bug in orafce, but this code worked last 7
> years.

That's very strange. It looks to me like you did exactly the right
thing. Can you provide any details on how it fails?

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

2016-02-12 14:10 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Fri, Feb 12, 2016 at 3:33 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> There will be necessary more changes than this. Orafce has some parts
> >> based on lw locks. I am able to compile it without any issue. But the
> lock
> >> mechanism is broken now - so impact on extensions will be higher. Have
> to do
> >> some research.
> >
> > if somebody would to use it for testing
> >
> > https://github.com/orafce/orafce
> >
> https://github.com/orafce/orafce/commit/fff56ed7e17ed5d6f8e6b71591ff1a6d6ff12d79
> >
> > With last commit I am able to compile orafce without warnings, but
> > installcheck is broken. It can be bug in orafce, but this code worked
> last 7
> > years.
>
> That's very strange. It looks to me like you did exactly the right
> thing. Can you provide any details on how it fails?
>

Looks like some race conditions is there - but I didn't tested it deeper

Pavel

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

On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> That's very strange. It looks to me like you did exactly the right
>> thing. Can you provide any details on how it fails?
>
> Looks like some race conditions is there - but I didn't tested it deeper

Well, OK, so I'm totally willing to work with you to help get this
straightened out, but I'm not really going to go download orafce and
debug it for you on company time. I'm fairly sure that won't win me
any large awards.

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

2016-02-12 15:43 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> That's very strange. It looks to me like you did exactly the right
> >> thing. Can you provide any details on how it fails?
> >
> > Looks like some race conditions is there - but I didn't tested it deeper
>
> Well, OK, so I'm totally willing to work with you to help get this
> straightened out, but I'm not really going to go download orafce and
> debug it for you on company time. I'm fairly sure that won't win me
> any large awards.
>

I'll do it - just need to finish some other. I hope so this night I'll know
more.

Regards

Pavel

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

2016-02-12 15:46 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2016-02-12 15:43 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:
>
>> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> >> That's very strange. It looks to me like you did exactly the right
>> >> thing. Can you provide any details on how it fails?
>> >
>> > Looks like some race conditions is there - but I didn't tested it deeper
>>
>> Well, OK, so I'm totally willing to work with you to help get this
>> straightened out, but I'm not really going to go download orafce and
>> debug it for you on company time. I'm fairly sure that won't win me
>> any large awards.
>>
>
> I'll do it - just need to finish some other. I hope so this night I'll
> know more.
>

In _PG_init I am creating new tranche by
RequestNamedLWLockTranche("orafce", 1);

Immediately when I try to use this lock

shmem_lock = sh_mem->shmem_lock = &(GetNamedLWLockTranche("orafce"))->lock;

I got a error

ERROR: XX000: requested tranche is not registered
LOCATION: GetNamedLWLockTranche, lwlock.c:602

Because the session initialization doesn't finish, then Orafce doesn't work

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

2016-02-12 17:35 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2016-02-12 15:46 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>
>>
>>
>> 2016-02-12 15:43 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>
>>> On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>>> wrote:
>>> >> That's very strange. It looks to me like you did exactly the right
>>> >> thing. Can you provide any details on how it fails?
>>> >
>>> > Looks like some race conditions is there - but I didn't tested it
>>> deeper
>>>
>>> Well, OK, so I'm totally willing to work with you to help get this
>>> straightened out, but I'm not really going to go download orafce and
>>> debug it for you on company time. I'm fairly sure that won't win me
>>> any large awards.
>>>
>>
>> I'll do it - just need to finish some other. I hope so this night I'll
>> know more.
>>
>
> In _PG_init I am creating new tranche by
> RequestNamedLWLockTranche("orafce", 1);
>
> Immediately when I try to use this lock
>
> shmem_lock = sh_mem->shmem_lock = &(GetNamedLWLockTranche("orafce"))->lock;
>
> I got a error
>
> ERROR: XX000: requested tranche is not registered
> LOCATION: GetNamedLWLockTranche, lwlock.c:602
>
> Because the session initialization doesn't finish, then Orafce doesn't work
>

I am starting to understand - the new design is more strict. The Orafce is
designed to run without registration shared_preload_libraries (it is
possible, but not necessary). But - RequestNamedLWLockTranche is working
only for this use case. Then GetNamedLWLockTranche fails, and all other are
probably consequences because shared memory isn't well initialized. After
setting shared_preload_libraries all tests are running. But I cannot do it
generally.

What is your recommendation for this case? So I have not to use named locks?

Regards

Pavel

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

On Fri, Feb 12, 2016 at 1:08 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I got a error
>>
>> ERROR: XX000: requested tranche is not registered
>> LOCATION: GetNamedLWLockTranche, lwlock.c:602
>>
>> Because the session initialization doesn't finish, then Orafce doesn't
>> work
>
> I am starting to understand - the new design is more strict. The Orafce is
> designed to run without registration shared_preload_libraries (it is
> possible, but not necessary). But - RequestNamedLWLockTranche is working
> only for this use case. Then GetNamedLWLockTranche fails, and all other are
> probably consequences because shared memory isn't well initialized. After
> setting shared_preload_libraries all tests are running. But I cannot do it
> generally.
>
> What is your recommendation for this case? So I have not to use named locks?

Hmm. So orafce is actually benefiting from the 3-lwlock slop that was
built into the old system: if one of those original 3 locks was
as-yet-unclaimed, orafce grabs it when it initializes. The new system
hasn't got that slop, and rather deliberately so. But that means that
the trick that worked before no longer works.

It looks to me like the easiest thing to do would be to change the
definition of sh_memory so that instead of containing an LWLockId, it
contains an LWLock and a tranche ID. Then the first process to do
ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
Every process, first or not, registers the tranche. Then you don't
need GetNamedLWLockTranche(). I think the problem right now is that
you can get the shared memory but fail to get the LWLock, and then you
have problems... if you put the LWLock in sh_memory, though, that
can't happen.

Of course, backward compatibility makes this a bit harder, but you
could do something like:

#if old-version
#define getlock(sh_mem) sh_mem->shmem_lock /* shmem_lock is an
LWLockId */
#else
#define getlock(sh_mem) &sh_mem->shmem_lock /* shmem_lock is an LWLock */
#endif

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

2016-02-13 4:52 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Fri, Feb 12, 2016 at 1:08 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> I got a error
> >>
> >> ERROR: XX000: requested tranche is not registered
> >> LOCATION: GetNamedLWLockTranche, lwlock.c:602
> >>
> >> Because the session initialization doesn't finish, then Orafce doesn't
> >> work
> >
> > I am starting to understand - the new design is more strict. The Orafce
> is
> > designed to run without registration shared_preload_libraries (it is
> > possible, but not necessary). But - RequestNamedLWLockTranche is working
> > only for this use case. Then GetNamedLWLockTranche fails, and all other
> are
> > probably consequences because shared memory isn't well initialized. After
> > setting shared_preload_libraries all tests are running. But I cannot do
> it
> > generally.
> >
> > What is your recommendation for this case? So I have not to use named
> locks?
>
> Hmm. So orafce is actually benefiting from the 3-lwlock slop that was
> built into the old system: if one of those original 3 locks was
> as-yet-unclaimed, orafce grabs it when it initializes. The new system
> hasn't got that slop, and rather deliberately so. But that means that
> the trick that worked before no longer works.
>
> It looks to me like the easiest thing to do would be to change the
> definition of sh_memory so that instead of containing an LWLockId, it
> contains an LWLock and a tranche ID. Then the first process to do
> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
> Every process, first or not, registers the tranche. Then you don't
> need GetNamedLWLockTranche(). I think the problem right now is that
> you can get the shared memory but fail to get the LWLock, and then you
> have problems... if you put the LWLock in sh_memory, though, that
> can't happen.
>

The Orafce design is based on old style of shared memory usage. It hasn't
own segment, and then the pointers are compatible between separate
processes. Using new style needs significant refactoring - and I would to
wait to end of support 9.3. Same situation is with LWLocks - I should to
share locks with Postmaster and doesn't create own tranche.

In previous design I was able to increase number of Postmaster locks, and
later, I can take one Postmaster lock. Is it possible?

Orafce is multi release code, and I would not to do significant refactoring
when I have not all necessary features on all supported releases. I
understand to fact, so Orafce uses obsolete design, but cannot to change in
this moment (and I would not it if it possible).

Regards

Pavel

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

On Sat, Feb 13, 2016 at 12:20 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> Hmm. So orafce is actually benefiting from the 3-lwlock slop that was
>> built into the old system: if one of those original 3 locks was
>> as-yet-unclaimed, orafce grabs it when it initializes. The new system
>> hasn't got that slop, and rather deliberately so. But that means that
>> the trick that worked before no longer works.
>>
>> It looks to me like the easiest thing to do would be to change the
>> definition of sh_memory so that instead of containing an LWLockId, it
>> contains an LWLock and a tranche ID. Then the first process to do
>> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
>> Every process, first or not, registers the tranche. Then you don't
>> need GetNamedLWLockTranche(). I think the problem right now is that
>> you can get the shared memory but fail to get the LWLock, and then you
>> have problems... if you put the LWLock in sh_memory, though, that
>> can't happen.
>
>
> The Orafce design is based on old style of shared memory usage. It hasn't
> own segment, and then the pointers are compatible between separate
> processes.

I'm not proposing that you switch to DSM. There's no real benefit to
that here. I'm just proposing that (at least in 9.5 and up) you put
the actual LWLock in the chunk of shared memory that you allocate,
rather than just an LWLockId/LWLock *.

> Using new style needs significant refactoring - and I would to
> wait to end of support 9.3. Same situation is with LWLocks - I should to
> share locks with Postmaster and doesn't create own tranche.
>
> In previous design I was able to increase number of Postmaster locks, and
> later, I can take one Postmaster lock. Is it possible?

Not unless we change something.

The actual code changes for what I proposed are not all that large.
But it is a certain amount of work and complexity that I understand is
not appealing to you.

We could back out these changes or invent some kind of backward
compatibility layer. I don't like that very much, but it could be
done.

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

2016-02-13 6:32 GMT+01:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Sat, Feb 13, 2016 at 12:20 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> Hmm. So orafce is actually benefiting from the 3-lwlock slop that was
> >> built into the old system: if one of those original 3 locks was
> >> as-yet-unclaimed, orafce grabs it when it initializes. The new system
> >> hasn't got that slop, and rather deliberately so. But that means that
> >> the trick that worked before no longer works.
> >>
> >> It looks to me like the easiest thing to do would be to change the
> >> definition of sh_memory so that instead of containing an LWLockId, it
> >> contains an LWLock and a tranche ID. Then the first process to do
> >> ShmemInitHash() can call LWLockNewTrancheId() and LWLockInitialize().
> >> Every process, first or not, registers the tranche. Then you don't
> >> need GetNamedLWLockTranche(). I think the problem right now is that
> >> you can get the shared memory but fail to get the LWLock, and then you
> >> have problems... if you put the LWLock in sh_memory, though, that
> >> can't happen.
> >
> >
> > The Orafce design is based on old style of shared memory usage. It hasn't
> > own segment, and then the pointers are compatible between separate
> > processes.
>
> I'm not proposing that you switch to DSM. There's no real benefit to
> that here. I'm just proposing that (at least in 9.5 and up) you put
> the actual LWLock in the chunk of shared memory that you allocate,
> rather than just an LWLockId/LWLock *.
>
> > Using new style needs significant refactoring - and I would to
> > wait to end of support 9.3. Same situation is with LWLocks - I should to
> > share locks with Postmaster and doesn't create own tranche.
> >
> > In previous design I was able to increase number of Postmaster locks, and
> > later, I can take one Postmaster lock. Is it possible?
>
> Not unless we change something.
>
> The actual code changes for what I proposed are not all that large.
> But it is a certain amount of work and complexity that I understand is
> not appealing to you.
>
> We could back out these changes or invent some kind of backward
> compatibility layer. I don't like that very much, but it could be
> done.
>

The compatibility layer can be great. Or the some simple API that allows to
use lock without hard code refactoring.

Regards

Pavel

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

On 10 February 2016 at 16:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
> wrote:
> >> (Sorry if this was discussed already, I haven't been paying attention)
> >>
> >> LWLockAssign() is used by extensions. Are we OK with just breaking them,
> >> requiring them to change LWLockAssign() with the new mechanism, with
> #ifdefs
> >> to support multiple server versions? Seems like it shouldn't be too
> hard to
> >> keep LWLockAssign() around for the benefit of extensions, so it seems a
> bit
> >> inconsiderate to remove it.
>
> > If there's a strong feeling that we should keep the old APIs around,
> > we can do that, but I think that (1) if we don't remove them now, we
> > probably never will and (2) they are vile APIs. Allocating the number
> > of add-in lwlocks that are requested or a minimum of 3 is just awful.
> > If somebody allocates a different number than they request it
> > sometimes works, except when combined with some other extension, when
> > it maybe doesn't work. This way, you ask for an LWLock under a given
> > name and then get it under that name, so if an extension does it
> > wrong, it is that extension that breaks rather than some other one. I
> > think that's enough benefit to justify requiring a small code change
> > on the part of extension authors that use LWLocks, but that's
> > obviously biased by my experience maintaining EDB's extensions, and
> > other people may well feel differently.
>
> FWIW, I wasn't paying attention either, but I'm convinced by Robert's
> argument. Avoiding coupling between extensions is worth an API break.
>

I don't buy that, sorry.

New features, new APIs, very much agreed. Better API is a reason for new
api, not a reason to remove old.

Old APIs - why can't we keep it? The change is minor, so it we can easily
map old to new. Why choose to break all extensions that do this? We could
easily keep this by making the old API assign locks out of a chunk called
"Old Extension API". Deprecate the old API and remove in a later release.
Like pretty much every other API we support.

We must respect that Extension authors need to support a number of our
releases, meaning their job is already complex enough. We want to support
people that write extensions, not throw obstacles in their path,

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On 10 February 2016 at 16:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> FWIW, I wasn't paying attention either, but I'm convinced by Robert's
>> argument. Avoiding coupling between extensions is worth an API break.

> Old APIs - why can't we keep it?

Because with the old API, a bug in extension A may go unnoticed in A's
testing but break when it's combined with extension B. That causes
headaches all around, not just to the extension authors but to their
users. The new API ensures detection of didn't-request-enough-locks
bugs regardless of which other extensions are installed. That is worth
the cost of a forced API update, in Robert's judgement and mine too.

(Having said that, I wonder if we could put back the old API as a shim
layer *without* the allocate-some-excess-locks proviso. That would
get us to a situation where standalone testing of a broken extension
would disclose its bug, without breaking non-buggy extensions.)

regards, tom lane

Hi

2016-02-13 15:54 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On 10 February 2016 at 16:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> FWIW, I wasn't paying attention either, but I'm convinced by Robert's
> >> argument. Avoiding coupling between extensions is worth an API break.
>
> > Old APIs - why can't we keep it?
>
> Because with the old API, a bug in extension A may go unnoticed in A's
> testing but break when it's combined with extension B. That causes
> headaches all around, not just to the extension authors but to their
> users. The new API ensures detection of didn't-request-enough-locks
> bugs regardless of which other extensions are installed. That is worth
> the cost of a forced API update, in Robert's judgement and mine too.
>
> (Having said that, I wonder if we could put back the old API as a shim
> layer *without* the allocate-some-excess-locks proviso. That would
> get us to a situation where standalone testing of a broken extension
> would disclose its bug, without breaking non-buggy extensions.)
>

If there will be simple way, how to fix it, then I'll fix my extensions.
But new API is working only when the extension has own share memory
segment. For some complex extensions like Orafce it means expensive
refactoring.

What is worst, this refactoring is difficult now, because I support older
versions when I have not private shared segments.

Regards

Pavel

> regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Tue, Apr 12, 2016 at 4:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> If there will be simple way, how to fix it, then I'll fix my extensions. But
> new API is working only when the extension has own share memory segment. For
> some complex extensions like Orafce it means expensive refactoring.
>
> What is worst, this refactoring is difficult now, because I support older
> versions when I have not private shared segments.

You don't need a separate shared memory segment. You might want to
have a look at what we did for pldebugger:

http://git.postgresql.org/gitweb/?p=pldebugger.git;a=commitdiff;h=14c6caf08bb14a7404a8241e47a80ef5f97e451e

I think the problem is similar to the one you are facing with orafce.

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

2016-04-12 14:51 GMT+02:00 Robert Haas <robertmhaas(at)gmail(dot)com>:

> On Tue, Apr 12, 2016 at 4:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > If there will be simple way, how to fix it, then I'll fix my extensions.
> But
> > new API is working only when the extension has own share memory segment.
> For
> > some complex extensions like Orafce it means expensive refactoring.
> >
> > What is worst, this refactoring is difficult now, because I support older
> > versions when I have not private shared segments.
>
> You don't need a separate shared memory segment. You might want to
> have a look at what we did for pldebugger:
>
>
> http://git.postgresql.org/gitweb/?p=pldebugger.git;a=commitdiff;h=14c6caf08bb14a7404a8241e47a80ef5f97e451e
>
> I think the problem is similar to the one you are facing with orafce.
>

I'll try it. Thank you for info

Pavel

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

2016-04-12 15:15 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2016-04-12 14:51 GMT+02:00 Robert Haas <robertmhaas(at)gmail(dot)com>:
>
>> On Tue, Apr 12, 2016 at 4:18 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
>> wrote:
>> > If there will be simple way, how to fix it, then I'll fix my
>> extensions. But
>> > new API is working only when the extension has own share memory
>> segment. For
>> > some complex extensions like Orafce it means expensive refactoring.
>> >
>> > What is worst, this refactoring is difficult now, because I support
>> older
>> > versions when I have not private shared segments.
>>
>> You don't need a separate shared memory segment. You might want to
>> have a look at what we did for pldebugger:
>>
>>
>> http://git.postgresql.org/gitweb/?p=pldebugger.git;a=commitdiff;h=14c6caf08bb14a7404a8241e47a80ef5f97e451e
>>
>> I think the problem is similar to the one you are facing with orafce.
>>
>
> I'll try it. Thank you for info
>

It is working. Thank you

Pavel

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

On Thu, Apr 14, 2016 at 2:00 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> You don't need a separate shared memory segment. You might want to
>>> have a look at what we did for pldebugger:
>>>
>>> http://git.postgresql.org/gitweb/?p=pldebugger.git;a=commitdiff;h=14c6caf08bb14a7404a8241e47a80ef5f97e451e
>>>
>>> I think the problem is similar to the one you are facing with orafce.
>>
>> I'll try it. Thank you for info
>
> It is working. Thank you

Thanks for confirming.

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