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

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