Ding-dong, contrib is dead ...

On 2006-09-05, Josh Berkus <josh@agliodbs.com> wrote:

Migrated to pgFoundry:

[...]

userlock (Merlin)

Userlock needs to go into core, not get removed; this was discussed in a
previous "let's clean up contrib/" thread. If it's not going into core
in 8.2 then it needs to stay in contrib.

People then didn't seem receptive to my suggestion that the existing
userlock code was non-copyrightable (on the grounds of being pure
interface), but that difficulty no longer exists now that Abhijit has
posted his clean-room rewrite (look for "otherlock" in -patches).
Perhaps he would be prepared to turn that into a patch against the core...

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

At 2006-09-05 05:47:58 -0000, andrew+nonews@supernews.com wrote:

that difficulty no longer exists now that Abhijit has posted his
clean-room rewrite (look for "otherlock" in -patches). Perhaps he
would be prepared to turn that into a patch against the core...

Absolutely. Just tell me where it should live and I'll post a patch.

-- ams

Josh Berkus <josh@agliodbs.com> writes:

Migrated to pgFoundry:
adddepends (Greg)
dbmirror (Steve Singer)
dbase --> dbf2pg
fulltextindex --> simplefti
mac (LER) --> mac-manufacturer
userlock (Merlin)

Please also kill the following two contrib directories, because despite an
impassioned plea by Robert Treat, they appear to not have any users (at least
nobody responded to posts on PWN or -hackers)
tips
mSQL-interface

Checking my copy of the "hit list", I thought we'd agreed to migrate
contrib/oracle as well.

regards, tom lane

Andrew - Supernews <andrew+nonews@supernews.com> writes:

Userlock needs to go into core, not get removed; this was discussed in a
previous "let's clean up contrib/" thread.

Something like it ought to go into core, but personally I'd opt for
taking the opportunity to redesign the API, which was a bit crufty to
begin with. That would eliminate all question of whether the clean room
was clean enough.

regards, tom lane

At 2006-09-05 10:23:19 -0400, tgl@sss.pgh.pa.us wrote:

Something like it ought to go into core, but personally I'd opt for
taking the opportunity to redesign the API, which was a bit crufty to
begin with.

I'm happy to do the work right away (not that there's much) if someone
suggests a better API. (I don't personally have a need for user-level
locks, and if I did, I'd be happy with just user_lock/user_unlock. So
if anyone has a more specific idea, I'm all ears.)

That would eliminate all question of whether the clean room was
clean enough.

It was really, really clean! Honest! :-)

-- ams

Tom Lane wrote:

Andrew - Supernews <andrew+nonews@supernews.com> writes:

Userlock needs to go into core, not get removed; this was discussed in a
previous "let's clean up contrib/" thread.

Something like it ought to go into core, but personally I'd opt for
taking the opportunity to redesign the API, which was a bit crufty to
begin with. That would eliminate all question of whether the clean room
was clean enough.

It seems odd to remove the module from contrib for 8.2 and then put a
replacement in core for 8.3. I guess we could signal our intentions in
the release notes.

It's a pity we didn't have Abhijit's patch 6 weeks ago.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

It's a pity we didn't have Abhijit's patch 6 weeks ago.

Well, now that we have it, the question is whether we want to do
anything with it. One problem is it lacks documentation.

However, as I said, I'd really rather choose a new API altogether. The
main thing that seems to be lacking is a way to wait for a lock, rather
than having only the equivalent of ConditionalLockAcquire. Also I don't
much like exposing a LOCKMODE directly to user code --- to use
user_lock() or user_unlock() you have to put magic numbers into your SQL
code and hope nobody reassigns the C enum values in future releases.
I'd be inclined to just expose the notions of "share" and "exclusive"
lock and make these separate functions instead of magic numbers.

And then there's the question of what to expose in the way of lock
identifier options. What we've got now is "two int4's or an OID"
which seems a bit random, not to mention that the key space overlaps
in an undocumented fashion. Possibly we could offer OID, int8, or
two int4s, and modify the code to set locktag_field4 to distinguish
these cases so that the key spaces are independent.

I have no opinions about function names, except that I'd suggest
choosing names based around "advisory lock" instead of "user lock".
Advisory locks are a standard concept and so that terminology
already tells people something ...

regards, tom lane

Andrew,

It seems odd to remove the module from contrib for 8.2 and then put a
replacement in core for 8.3. I guess we could signal our intentions in
the release notes.

The current code is GPL. It *has* to be removed.

--Josh

Tom,

Checking my copy of the "hit list", I thought we'd agreed to migrate
contrib/oracle as well.

Hmmm ... somehow that got dropped out of discussions early on, without
any reason why. See the "more nuclear options" thread; oracle is
nowhere on it.

Will only take me 30 min to migrate, but we need to give people time to
object ... and I need to check whether there's even any code in there
that superceded orasysviews and orafce and oralink.

Hey, everyone, if you have a reason for contrib/oracle not to migrate
out to pgFoundry, speak up now!

Or if you want to take the new project over, speak up too.

--Josh Berkus

Josh Berkus <josh@agliodbs.com> writes:

Checking my copy of the "hit list", I thought we'd agreed to migrate
contrib/oracle as well.

Hmmm ... somehow that got dropped out of discussions early on, without
any reason why.

Actually ... never mind that, it seems to have been done already.
Sorry for the noise.

regards, tom lane

On 9/5/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

It's a pity we didn't have Abhijit's patch 6 weeks ago.

Well, now that we have it, the question is whether we want to do
anything with it. One problem is it lacks documentation.

yes, userlocks have to be documented, in particular the dangers of
lock exhaustion. also a tie-in to the recently upgraded pg_locks view
would be nice.

However, as I said, I'd really rather choose a new API altogether. The
main thing that seems to be lacking is a way to wait for a lock, rather
than having only the equivalent of ConditionalLockAcquire. Also I don't
much like exposing a LOCKMODE directly to user code --- to use
user_lock() or user_unlock() you have to put magic numbers into your SQL
code and hope nobody reassigns the C enum values in future releases.
I'd be inclined to just expose the notions of "share" and "exclusive"
lock and make these separate functions instead of magic numbers.

I agree 100%.

And then there's the question of what to expose in the way of lock
identifier options. What we've got now is "two int4's or an OID"
which seems a bit random, not to mention that the key space overlaps
in an undocumented fashion. Possibly we could offer OID, int8, or
two int4s, and modify the code to set locktag_field4 to distinguish
these cases so that the key spaces are independent.

right, this is some legacy cruft, in fact I raised this to your
attention which was perhaps part of the inspiration to upgrade
pg_locks.

I have no opinions about function names, except that I'd suggest
choosing names based around "advisory lock" instead of "user lock".
Advisory locks are a standard concept and so that terminology
already tells people something ...

Agreement here also.

As to the point of userlocks being in core, they are in fact already
in core, and have been several years, the name 'userlock' having been
taken from the in source documentation. The userlock contrib module
is nothing besides some wrappers for exisiting functions built into
the backend. Removing userlock from contrib just removes a convenient
mechanism to use them.

I also agree with Andrew that pgfoundry is not a appropriate place for
userlocks. They should be properly documented with a cleaned up api.
I have no objection from them being removed from contrib in the short
term due to the gpl issue, although I am not sure how you can
copyright a function wrapper.

merlin

"Merlin Moncure" <mmoncure@gmail.com> writes:

I also agree with Andrew that pgfoundry is not a appropriate place for
userlocks. They should be properly documented with a cleaned up api.
I have no objection from them being removed from contrib in the short
term due to the gpl issue, although I am not sure how you can
copyright a function wrapper.

Right, I see the pgfoundry project as just a backwards-compatibility
thing for anyone who doesn't want to change their code. I'm happy to
put some cleaned-up functions into core right now (ie, for 8.2) if
someone will do the legwork to define and implement them.

After further thought it occurs to me that having both OID and int8
keys might be a problem, in that it's not too clear which you'd get
from a single-argument call. But we could offer just int8 and two-int4
signatures and rely on promoting OID to int8 if you need a lock on OID.

So the function list might look like

void pg_advisory_lock(int8) wait
void pg_advisory_lock_shared(int8) wait
bool pg_try_advisory_lock(int8) no wait
bool pg_try_advisory_lock_shared(int8) no wait
bool pg_advisory_unlock(int8) returns T if successful
bool pg_advisory_unlock_shared(int8) returns T if successful

plus all the above taking 2 int4's, plus

void pg_advisory_unlock_all()

Not wedded to these names at all...

regards, tom lane

On 2006-09-05, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

It's a pity we didn't have Abhijit's patch 6 weeks ago.

Well, now that we have it, the question is whether we want to do
anything with it. One problem is it lacks documentation.

However, as I said, I'd really rather choose a new API altogether.

What about existing users?

The main thing that seems to be lacking is a way to wait for a lock,

Is this a feature that people actually want or need?

Certainly exposing the lockmode as a magic number isn't ideal.

And then there's the question of what to expose in the way of lock
identifier options. What we've got now is "two int4's or an OID"
which seems a bit random, not to mention that the key space overlaps
in an undocumented fashion.

It is documented in the original README.user_locks.

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

On 2006-09-05, Josh Berkus <josh@agliodbs.com> wrote:

The current code is GPL. It *has* to be removed.

Which is why Abhijit's version exists - it's intended to be a drop-in,
BSD-licensed replacement for the current code.

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

On 9/5/06, Andrew - Supernews <andrew+nonews@supernews.com> wrote:

On 2006-09-05, Josh Berkus <josh@agliodbs.com> wrote:

The current code is GPL. It *has* to be removed.

Which is why Abhijit's version exists - it's intended to be a drop-in,
BSD-licensed replacement for the current code.

does his patch include documentation? I can help with that if it isn't
done. was it reviewed?

merlin

Merlin,

The current code is GPL. It *has* to be removed.

Which is why Abhijit's version exists - it's intended to be a drop-in,
BSD-licensed replacement for the current code.

does his patch include documentation? I can help with that if it isn't
done. was it reviewed?

No, and no. It's unfortunately too late for Abhijit's patch to make it
into 8.2; it was't submitted until last week, I believe.

So userlocks will be in pgFoundry for the next rev -- frankly, it should
have been for 8.1 but I forgot it. For the 8.3 version, as Tom has
indicated we may want to change the API somewhat anyway, so we'll want the
pgFoundry version for backwards-compat.

--
--Josh

Josh Berkus
PostgreSQL @ Sun
San Francisco

Tom Lane wrote:

Checking my copy of the "hit list", I thought we'd agreed to migrate
contrib/oracle as well.

It has already been removed because it is being actively maintained
elsewhere.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On 9/5/06, Josh Berkus <josh@agliodbs.com> wrote:

So userlocks will be in pgFoundry for the next rev -- frankly, it should
have been for 8.1 but I forgot it. For the 8.3 version, as Tom has
indicated we may want to change the API somewhat anyway, so we'll want the
pgFoundry version for backwards-compat.

well, I'm confused now. Tom said that cleaned up functions might be
sneaked into 8.2, which is what prompted my question. If that's the
case I'm considering putting something together quickly. It's no big
deal to me either way really. However, it would really be a shame to
drop the contrib module and leave 8.2 without a way of easily use them
(userlocks being, imho, the #1 greatest undiscovered feature in pg).

To be honest, I don't see the need for a backwards-compat version at
all, because all you need to do is copy and paste the code from 8.1.
If advisory functions are promoted in core (8.3 or no), sql wrappers
for compatibility would be trivial to implement. These are one line
wrappers here.

merlin

On 2006-09-05, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Right, I see the pgfoundry project as just a backwards-compatibility
thing for anyone who doesn't want to change their code. I'm happy to
put some cleaned-up functions into core right now (ie, for 8.2) if
someone will do the legwork to define and implement them.

So you're prepared to violate the feature freeze to stick in a new API
that nobody currently wants to _use_, while forcing existing users to
resort to pgfoundry for a module that's been around for several major
releases?

--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services