Thread: Re: [COMMITTERS] pgsql: Code cleanup in the wake of recent LWLock refactoring.

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.

On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka@iki.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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.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.

Regards

Pavel

 

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

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.

On Fri, Feb 12, 2016 at 2:03 PM, Pavel Stehule <pavel.stehule@gmail.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

On Fri, Feb 12, 2016 at 3:33 AM, Pavel Stehule <pavel.stehule@gmail.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

On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel.stehule@gmail.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@gmail.com>:

On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel.stehule@gmail.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.stehule@gmail.com>:

2016-02-12 15:43 GMT+01:00 Robert Haas <robertmhaas@gmail.com>:

On Fri, Feb 12, 2016 at 8:16 AM, Pavel Stehule <pavel.stehule@gmail.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

On Fri, Feb 12, 2016 at 1:08 PM, Pavel Stehule <pavel.stehule@gmail.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.

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

On Sat, Feb 13, 2016 at 12:20 AM, Pavel Stehule <pavel.stehule@gmail.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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Feb 10, 2016 at 11:08 AM, Heikki Linnakangas <hlinnaka@iki.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.

On Tue, Apr 12, 2016 at 4:18 AM, Pavel Stehule <pavel.stehule@gmail.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@gmail.com>:

On Tue, Apr 12, 2016 at 4:18 AM, Pavel Stehule <pavel.stehule@gmail.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