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

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
>