Re: [PATCH] Replacement for OSSP-UUID for Linux and BSD

Hi everyone,

here's the latest version of my uuid changes patch, according to
proposal (2) from Tom in the thread about OSSP-UUID[1].

* it works on linux and bsd (tested on ubuntu and netbsd), using their
own uuid implementations
* the configure switch is now --with-uuid
* function signatures are fully compatible with ossp-uuid
* it has some regression tests

The latest version of the patch also fixes some issues wrt RFC
compliance as I've realised the RFC itself had a wrong example[2]. The
output now matches the one from OSSP-UUID, although I couldn't verify
the behaviour on big-endian systems.

Feel free to consider it for 9.4b2 or the next commit fest.

[1] http://www.postgresql.org/message-id/9285.1400387306@sss.pgh.pa.us
[2] http://www.rfc-editor.org/errata_search.php?rfc=4122

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Matteo Beccati <php(at)beccati(dot)com> writes:
> here's the latest version of my uuid changes patch, according to
> proposal (2) from Tom in the thread about OSSP-UUID[1].

Hmm ... this is not actually what I had in mind. Unless I'm misreading
the patch, this nukes the "uuid-ossp" extension entirely in favor of a
new extension "uuid" (providing the same SQL functions with a different
underlying implementation). I don't think this works from the standpoint
of providing compatibility for users of the existing extension.
In particular, it'd break pg_upgrade (because of the change of the .so
library name) as well as straight pg_dump upgrades (which would expect
CREATE EXTENSION "uuid-ossp" to work). Not to mention application code
that might expect CREATE EXTENSION "uuid-ossp" to still work.

Another objection is that for people for whom OSSP uuid still works fine,
this is forcing them to adopt a new implementation whose compatibility is
as yet unproven.

What I'd rather do is preserve contrib/uuid-ossp with the same extension
and .so name, but with two configure options that select different
underlying implementations.

In the long run it'd be nice to migrate away from the "uuid-ossp"
extension name, mostly because of the poorly-chosen use of a dash in the
name. But I'm not sure how we do that without breaking backwards
compatibility, and anyway it's an entirely cosmetic thing that we can
worry about later.

Anyhow, doing it like that seems like it ought to be a pretty
straightforward refactoring of your patch. I could pursue that,
or you can.

regards, tom lane

Hi Tom,

thanks for the feedback.

On 25/05/2014 21:10, Tom Lane wrote:
> Matteo Beccati <php(at)beccati(dot)com> writes:
>> here's the latest version of my uuid changes patch, according to
>> proposal (2) from Tom in the thread about OSSP-UUID[1].
>
> Hmm ... this is not actually what I had in mind. Unless I'm misreading
> the patch, this nukes the "uuid-ossp" extension entirely in favor of a
> new extension "uuid" (providing the same SQL functions with a different
> underlying implementation). I don't think this works from the standpoint
> of providing compatibility for users of the existing extension.
> In particular, it'd break pg_upgrade (because of the change of the .so
> library name) as well as straight pg_dump upgrades (which would expect
> CREATE EXTENSION "uuid-ossp" to work). Not to mention application code
> that might expect CREATE EXTENSION "uuid-ossp" to still work.
>
> Another objection is that for people for whom OSSP uuid still works fine,
> this is forcing them to adopt a new implementation whose compatibility is
> as yet unproven.
>
> What I'd rather do is preserve contrib/uuid-ossp with the same extension
> and .so name, but with two configure options that select different
> underlying implementations.

Sure, that makes sense. I wasn't actually sure it was ok to keep the
"OSSP" brand even though the extensions didn't use the oosp library,
hence the renaming. But I do agree upgrades wouldn't be very easy if we
don't.

> In the long run it'd be nice to migrate away from the "uuid-ossp"
> extension name, mostly because of the poorly-chosen use of a dash in the
> name. But I'm not sure how we do that without breaking backwards
> compatibility, and anyway it's an entirely cosmetic thing that we can
> worry about later.
>
> Anyhow, doing it like that seems like it ought to be a pretty
> straightforward refactoring of your patch. I could pursue that,
> or you can.

I do have a system with the ossp library installed: I'd be happy to give
it a try tomorrow morning my time, depending on my workload. I'll keep
you posted!

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Hi Tom,

On 25/05/2014 21:10, Tom Lane wrote:
> Anyhow, doing it like that seems like it ought to be a pretty
> straightforward refactoring of your patch. I could pursue that,
> or you can.

I'm attaching v2 of the patch. Here's a list of changes from v1:

* Restored --with-ossp-uuid. Configure tries ossp support first, then
falls back to Linux and BSD variants
* md5.o and sha1.o are linked only when not using the ossp library
* fixed a bug in the v1mc generation carried over from uuid-freebsd:
only the mac part is randomized now
* updated regression tests, which are now passing on ubuntu w/ and w/o
ossp and netbsd.

This was the best I could achieve in the limited timeframe I had
available. I believe it could be a good starting point for your final
refactoring.

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Matteo Beccati <php(at)beccati(dot)com> writes:
> I'm attaching v2 of the patch. Here's a list of changes from v1:

> * Restored --with-ossp-uuid. Configure tries ossp support first, then
> falls back to Linux and BSD variants
> * md5.o and sha1.o are linked only when not using the ossp library
> * fixed a bug in the v1mc generation carried over from uuid-freebsd:
> only the mac part is randomized now
> * updated regression tests, which are now passing on ubuntu w/ and w/o
> ossp and netbsd.

> This was the best I could achieve in the limited timeframe I had
> available. I believe it could be a good starting point for your final
> refactoring.

I've not reviewed this in any detail yet, but I believe we're in agreement
on the general ideas. I do think that we need two separate configure
switches: one asking to use the OSSP library and one asking to use the
other library. That's an easy change though.

Assuming this works as advertised, does anyone have an objection to
pushing it into 9.4?

regards, tom lane

On 2014-05-26 13:25:49 -0400, Tom Lane wrote:
> Matteo Beccati <php(at)beccati(dot)com> writes:
> > I'm attaching v2 of the patch. Here's a list of changes from v1:
>
> > * Restored --with-ossp-uuid. Configure tries ossp support first, then
> > falls back to Linux and BSD variants

Imo should be the other way round.

> > * md5.o and sha1.o are linked only when not using the ossp library

The backend already has a md5 implementation, no?

> Assuming this works as advertised, does anyone have an objection to
> pushing it into 9.4?

I think it's a pretty bad idea. But the alternatives seem worse. We
should better get it right though if we do it.

Greetings,

Andres Freund

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

On 26/05/2014 19:31, Andres Freund wrote:
> On 2014-05-26 13:25:49 -0400, Tom Lane wrote:
>> Matteo Beccati <php(at)beccati(dot)com> writes:
>>> I'm attaching v2 of the patch. Here's a list of changes from v1:
>>
>>> * Restored --with-ossp-uuid. Configure tries ossp support first, then
>>> falls back to Linux and BSD variants
>
> Imo should be the other way round.

My goal was to avoid changing the underlying implementation if someone
has the ossp library installed. Tom is suggesting to split the configure
switch in two, so I guess that would allow the most flexibility.

>>> * md5.o and sha1.o are linked only when not using the ossp library
>
> The backend already has a md5 implementation, no?

Yes, but as far as I could see it can only convert a string to hash.
Making it fit the uuid use case would require concatenating the
namespace and source string before passing it to the md5 function. The
BSD system and pgcrypto implementations do not:

https://github.com/mbeccati/postgres/blob/ossp/contrib/uuid-ossp/uuid-ossp.c#L237

We could even use the system md5 and sha1 on BSD, but I've spotted at
least a difference between FreeBSD and NetBSD (sha.h vs sha1.h and
different function names) and I didn't think it was worth to pursue
that, especially if beta2 is the target.

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Matteo Beccati <php(at)beccati(dot)com> writes:
> On 26/05/2014 19:31, Andres Freund wrote:
>> On 2014-05-26 13:25:49 -0400, Tom Lane wrote:
>>> Matteo Beccati <php(at)beccati(dot)com> writes:
>>>> * Restored --with-ossp-uuid. Configure tries ossp support first, then
>>>> falls back to Linux and BSD variants

>> Imo should be the other way round.

> My goal was to avoid changing the underlying implementation if someone
> has the ossp library installed. Tom is suggesting to split the configure
> switch in two, so I guess that would allow the most flexibility.

Yeah: if we have two configure switches then we can avoid having to decide
which library takes precedence. But the main reason for that is we have
a general policy that what you get from a given configure switch should be
predictable, rather than depending on what is or is not installed. (If we
were sure these libraries were 100% interchangeable then maybe one switch
would be OK, but I'm not too sure about that, yet.)

regards, tom lane

Re: Tom Lane 2014-05-25 <12508(dot)1401045013(at)sss(dot)pgh(dot)pa(dot)us>
> Matteo Beccati <php(at)beccati(dot)com> writes:
> > here's the latest version of my uuid changes patch, according to
> > proposal (2) from Tom in the thread about OSSP-UUID[1].
>
> Hmm ... this is not actually what I had in mind. Unless I'm misreading
> the patch, this nukes the "uuid-ossp" extension entirely in favor of a
> new extension "uuid" (providing the same SQL functions with a different
> underlying implementation). I don't think this works from the standpoint
> of providing compatibility for users of the existing extension.
> In particular, it'd break pg_upgrade (because of the change of the .so
> library name) as well as straight pg_dump upgrades (which would expect
> CREATE EXTENSION "uuid-ossp" to work). Not to mention application code
> that might expect CREATE EXTENSION "uuid-ossp" to still work.
>
> Another objection is that for people for whom OSSP uuid still works fine,
> this is forcing them to adopt a new implementation whose compatibility is
> as yet unproven.
>
> What I'd rather do is preserve contrib/uuid-ossp with the same extension
> and .so name, but with two configure options that select different
> underlying implementations.

What I'd propose is to have a "pure" uuid extension using native OS
functions, so people starting a new database could go for that without
any OSSP legacy. The ossp-uuid extension would then have a dependency
(like earthdistance has requires = 'cube'), plus maybe compatibility
functions to call the proper functions from uuid.

This would make pg_dump upgrades work. No idea about pg_upgrade, but I
wanted to post that idea here, as it didn't seem to have been
discussed yet.

> In the long run it'd be nice to migrate away from the "uuid-ossp"
> extension name, mostly because of the poorly-chosen use of a dash in the
> name. But I'm not sure how we do that without breaking backwards
> compatibility, and anyway it's an entirely cosmetic thing that we can
> worry about later.
>
> Anyhow, doing it like that seems like it ought to be a pretty
> straightforward refactoring of your patch. I could pursue that,
> or you can.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

Christoph Berg <cb(at)df7cb(dot)de> writes:
> Re: Tom Lane 2014-05-25 <12508(dot)1401045013(at)sss(dot)pgh(dot)pa(dot)us>
>> Hmm ... this is not actually what I had in mind. Unless I'm misreading
>> the patch, this nukes the "uuid-ossp" extension entirely in favor of a
>> new extension "uuid" (providing the same SQL functions with a different
>> underlying implementation). I don't think this works from the standpoint
>> of providing compatibility for users of the existing extension.
>> In particular, it'd break pg_upgrade (because of the change of the .so
>> library name) as well as straight pg_dump upgrades (which would expect
>> CREATE EXTENSION "uuid-ossp" to work). Not to mention application code
>> that might expect CREATE EXTENSION "uuid-ossp" to still work.
>>
>> Another objection is that for people for whom OSSP uuid still works fine,
>> this is forcing them to adopt a new implementation whose compatibility is
>> as yet unproven.
>>
>> What I'd rather do is preserve contrib/uuid-ossp with the same extension
>> and .so name, but with two configure options that select different
>> underlying implementations.

> What I'd propose is to have a "pure" uuid extension using native OS
> functions, so people starting a new database could go for that without
> any OSSP legacy. The ossp-uuid extension would then have a dependency
> (like earthdistance has requires = 'cube'), plus maybe compatibility
> functions to call the proper functions from uuid.

The only thing that seems particularly "legacy" about this design is that
the extension is still named uuid-ossp. While that's certainly annoying
when the implementation isn't using OSSP, I'm not really seeing the value
of providing two near-duplicate extensions. And no, what you suggest
doesn't work well with pg_upgrade. I'm not even sure it works well
ignoring that issue: the new extension would have to choose all-new
function names so that it could be installed alongside uuid-ossp.

regards, tom lane

I wrote:
> Matteo Beccati <php(at)beccati(dot)com> writes:
>> On 26/05/2014 19:31, Andres Freund wrote:
>>> On 2014-05-26 13:25:49 -0400, Tom Lane wrote:
>>>> Matteo Beccati <php(at)beccati(dot)com> writes:
>>>>> * Restored --with-ossp-uuid. Configure tries ossp support first, then
>>>>> falls back to Linux and BSD variants

>>> Imo should be the other way round.

>> My goal was to avoid changing the underlying implementation if someone
>> has the ossp library installed. Tom is suggesting to split the configure
>> switch in two, so I guess that would allow the most flexibility.

> Yeah: if we have two configure switches then we can avoid having to decide
> which library takes precedence. But the main reason for that is we have
> a general policy that what you get from a given configure switch should be
> predictable, rather than depending on what is or is not installed. (If we
> were sure these libraries were 100% interchangeable then maybe one switch
> would be OK, but I'm not too sure about that, yet.)

Some research uncovered the fact that the e2fsprogs UUID library (ie the
"Linux" one) is available for FreeBSD too:
http://www.freshports.org/misc/e2fsprogs-libuuid/

This means that if we want to give users control over which implementation
gets selected, we actually need *three* configure switches. In the
attached revision of Matteo's patch, I called them --with-ossp-uuid
(the existing switch name), --with-linux-uuid, and --with-bsd-uuid.
I'm not necessarily wedded to the latter two names; in particular it seems
unfortunate that the right one to use on OS X is --with-linux-uuid.
But I think --with-e2fsprogs-uuid is right out, so it's not clear what
would be better.

I've verified functionality of this patch on these scenarios:

(1) --with-ossp-uuid on RHEL6, using uuid-1.6.1-10.el6.x86_64
(2) --with-linux-uuid on RHEL6, using libuuid-2.17.2-12.14.el6_5.x86_64
(3) --with-linux-uuid on OS X 10.9.3, Intel
(4) --with-linux-uuid on OS X 10.4.11, PPC (hence, bigendian)

I do not have a machine on which to try --with-bsd-uuid, so it's
possible I broke that portion of Matteo's patch. Would someone try
that case on a FreeBSD box?

Other than retesting that case, I think this is ready to go except that
it lacks documentation updates.

regards, tom lane

On May 26, 2014, at 6:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> This means that if we want to give users control over which implementation
> gets selected, we actually need *three* configure switches. In the
> attached revision of Matteo's patch, I called them --with-ossp-uuid
> (the existing switch name), --with-linux-uuid, and --with-bsd-uuid.
> I'm not necessarily wedded to the latter two names; in particular it seems
> unfortunate that the right one to use on OS X is --with-linux-uuid.
> But I think --with-e2fsprogs-uuid is right out, so it's not clear what
> would be better.

How about --with-unix-uuid? Or --with-ext2-uuid?

Which one is the default -- or is there one? Should we use some sort of mapping to select the right switch by platform, or if ossp-uuid appears to be installed?

Best,

David

"David E. Wheeler" <david(at)justatheory(dot)com> writes:
> On May 26, 2014, at 6:07 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This means that if we want to give users control over which implementation
>> gets selected, we actually need *three* configure switches. In the
>> attached revision of Matteo's patch, I called them --with-ossp-uuid
>> (the existing switch name), --with-linux-uuid, and --with-bsd-uuid.
>> I'm not necessarily wedded to the latter two names; in particular it seems
>> unfortunate that the right one to use on OS X is --with-linux-uuid.
>> But I think --with-e2fsprogs-uuid is right out, so it's not clear what
>> would be better.

> How about --with-unix-uuid? Or --with-ext2-uuid?

Meh. "Unix" certainly subsumes BSD, so that doesn't seem like a very
useful distinction. I guess we could use "ext2" but that would just
confuse most people.

> Which one is the default -- or is there one?

The point here is that we won't make a default choice.

regards, tom lane

On May 26, 2014, at 9:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>> How about --with-unix-uuid? Or --with-ext2-uuid?
>
> Meh. "Unix" certainly subsumes BSD, so that doesn't seem like a very
> useful distinction. I guess we could use "ext2" but that would just
> confuse most people.

--with-uuid?

>> Which one is the default -- or is there one?
>
> The point here is that we won't make a default choice.

So no UUID functions by default, which I guess has been the case all along?

Always seemed weird to me that there was a core configure option specific to a contrib module.

D

"David E. Wheeler" <david(at)justatheory(dot)com> writes:
> On May 26, 2014, at 9:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> How about --with-unix-uuid? Or --with-ext2-uuid?

>> Meh. "Unix" certainly subsumes BSD, so that doesn't seem like a very
>> useful distinction. I guess we could use "ext2" but that would just
>> confuse most people.

> --with-uuid?

I thought about that but figured we'd regret it ...

> So no UUID functions by default, which I guess has been the case all along?

Yup.

> Always seemed weird to me that there was a core configure option specific to a contrib module.

There's other cases, eg --with-libxslt for contrib/xml2. I think
contrib/sepgsql has bespoke configure support too.

Fundamentally the reason uuid-ossp isn't in core is that we feared it
wasn't portable enough, a fear subsequently thoroughly validated.
Perhaps this patch will move us a notch towards having functionality
we could expect to be available everywhere ... though we're certainly
not all the way there yet.

regards, tom lane

Hi Tom,

On 27/05/2014 03:07, Tom Lane wrote:
> I've verified functionality of this patch on these scenarios:
>
> (1) --with-ossp-uuid on RHEL6, using uuid-1.6.1-10.el6.x86_64
> (2) --with-linux-uuid on RHEL6, using libuuid-2.17.2-12.14.el6_5.x86_64
> (3) --with-linux-uuid on OS X 10.9.3, Intel
> (4) --with-linux-uuid on OS X 10.4.11, PPC (hence, bigendian)
>
> I do not have a machine on which to try --with-bsd-uuid, so it's
> possible I broke that portion of Matteo's patch. Would someone try
> that case on a FreeBSD box?

I've tested on NetBSD i386 and --with-bsd-uuid worked out of the box. I
could fire up some virtual machines with FreeBSD and other BSD flavours,
but maybe some buildfarm animals could be used for that.

I'm attaching a little patch to be applied on top of yours.

I didn't notice that "buf ? 13 : 0" was raising a warning about the
condition being always true on BSD. I guess it's safe to hardcode 13 as
the argument is ignored anyway with ossp, so I've fixed that.

I've also fixed v1mc generation on "linux" to match the OSSP and BSD
variants and added a regression test for it.

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Matteo Beccati <php(at)beccati(dot)com> writes:
> On 27/05/2014 03:07, Tom Lane wrote:
>> I do not have a machine on which to try --with-bsd-uuid, so it's
>> possible I broke that portion of Matteo's patch. Would someone try
>> that case on a FreeBSD box?

> I've tested on NetBSD i386 and --with-bsd-uuid worked out of the box.

Ah, cool. I had documented this option as only working for FreeBSD,
but that's obviously too conservative. Anyone know about whether it
works on OpenBSD?

> I'm attaching a little patch to be applied on top of yours.

Thanks, will incorporate this.

regards, tom lane

Hi Tom,

On 27/05/2014 15:52, Tom Lane wrote:
> Matteo Beccati <php(at)beccati(dot)com> writes:
>> On 27/05/2014 03:07, Tom Lane wrote:
>>> I do not have a machine on which to try --with-bsd-uuid, so it's
>>> possible I broke that portion of Matteo's patch. Would someone try
>>> that case on a FreeBSD box?
>
>> I've tested on NetBSD i386 and --with-bsd-uuid worked out of the box.
>
> Ah, cool. I had documented this option as only working for FreeBSD,
> but that's obviously too conservative. Anyone know about whether it
> works on OpenBSD?

I've tried to google "man uuid openbsd" and I got the e2fs package
(which contains uuid/uuid.h and libuuid) instead of a man page, so I
believe that could be another use case for --with-linux-uuid.

If it's confirmed to be working, that makes two BSD-derived systems
requiring "linux-uuid", so --with-e2fs-uuid or similar would be more
appropriate.

Cheers
--
Matteo Beccati

Development & Consulting - http://www.beccati.com/

Tom Lane wrote:
> "David E. Wheeler" <david(at)justatheory(dot)com> writes:
> > On May 26, 2014, at 9:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >>> How about --with-unix-uuid? Or --with-ext2-uuid?
>
> >> Meh. "Unix" certainly subsumes BSD, so that doesn't seem like a very
> >> useful distinction. I guess we could use "ext2" but that would just
> >> confuse most people.
>
> > --with-uuid?
>
> I thought about that but figured we'd regret it ...

Why not --with-uuid-implementation=<impl>, and have <impl> be one of
e2utils, bsd, ossp, with the latter being default? We could also have
offer the value "list" or "help" which would list the available options.
That way, if we come up with a new implementation in the future, this is
easily extensible.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Matteo Beccati <php(at)beccati(dot)com> writes:
> On 27/05/2014 15:52, Tom Lane wrote:
>> Ah, cool. I had documented this option as only working for FreeBSD,
>> but that's obviously too conservative. Anyone know about whether it
>> works on OpenBSD?

> I've tried to google "man uuid openbsd" and I got the e2fs package
> (which contains uuid/uuid.h and libuuid) instead of a man page, so I
> believe that could be another use case for --with-linux-uuid.

Yeah, googling led me to the same conclusion: e2fsprogs libuuid is
definitely available for OpenBSD, but I found no evidence that it
has the built-in UUID functions that FreeBSD/NetBSD have.

> If it's confirmed to be working, that makes two BSD-derived systems
> requiring "linux-uuid", so --with-e2fs-uuid or similar would be more
> appropriate.

Still don't like that name much, but maybe that's the best we can do.

regards, tom lane

On 2014-05-27 16:36:45 +0200, Matteo Beccati wrote:
> On 27/05/2014 15:52, Tom Lane wrote:
> > Ah, cool. I had documented this option as only working for FreeBSD,
> > but that's obviously too conservative. Anyone know about whether it
> > works on OpenBSD?
>
> I've tried to google "man uuid openbsd" and I got the e2fs package
> (which contains uuid/uuid.h and libuuid) instead of a man page, so I
> believe that could be another use case for --with-linux-uuid.
>
> If it's confirmed to be working, that makes two BSD-derived systems
> requiring "linux-uuid", so --with-e2fs-uuid or similar would be more
> appropriate.

When I looked at the e2fs uuid implementation a year or so back, it even
had some form of windows support. So +1 to naming it non-os specifically.

Greetings,

Andres Freund

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

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Why not --with-uuid-implementation=<impl>, and have <impl> be one of
> e2utils, bsd, ossp, with the latter being default? We could also have
> offer the value "list" or "help" which would list the available options.
> That way, if we come up with a new implementation in the future, this is
> easily extensible.

The problem is that the long-established spelling is --with-ossp-uuid.
I don't think we can break that case. While we could set up something
like what you propose alongside it, it doesn't seem like there's any
advantage to doing so compared to inventing --with-foo-uuid as needed.

In either case, the problem remains of exactly what to call the
e2fsprogs-derived implementation. It does seem that people who are
familiar with these libraries call it that, but I'm worried that such
a name will confuse those not so familiar.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> > Why not --with-uuid-implementation=<impl>, and have <impl> be one of
> > e2utils, bsd, ossp, with the latter being default? We could also have
> > offer the value "list" or "help" which would list the available options.
> > That way, if we come up with a new implementation in the future, this is
> > easily extensible.
>
> The problem is that the long-established spelling is --with-ossp-uuid.
> I don't think we can break that case. While we could set up something
> like what you propose alongside it, it doesn't seem like there's any
> advantage to doing so compared to inventing --with-foo-uuid as needed.

I was thinking that --with-ossp-uuid would still be required to enable
UUID generators at all; the other one just selects the implementation to
use, which defaults to OSSP to maintain backwards compatibility. Maybe
introduce --with-uuid and have --with-ossp-uuid a deprecated synonym of
that.

As a more sophisticated idea: if you say --with-uuid then
--with-uuid-implementation is required; if you say --with-ossp-uuid then
--with-uuid-implementation is optional and defaults to OSSP.

> In either case, the problem remains of exactly what to call the
> e2fsprogs-derived implementation. It does seem that people who are
> familiar with these libraries call it that, but I'm worried that such
> a name will confuse those not so familiar.

I vote "e2fsprogs". In the help text for that option, mention that it
works on Linux and some others OSes, or something like that so that
people on Linux try that one first, and people on other OSes can
web-search to see whether it's available.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> Tom Lane wrote:
>> The problem is that the long-established spelling is --with-ossp-uuid.
>> I don't think we can break that case. While we could set up something
>> like what you propose alongside it, it doesn't seem like there's any
>> advantage to doing so compared to inventing --with-foo-uuid as needed.

> I was thinking that --with-ossp-uuid would still be required to enable
> UUID generators at all; the other one just selects the implementation to
> use, which defaults to OSSP to maintain backwards compatibility. Maybe
> introduce --with-uuid and have --with-ossp-uuid a deprecated synonym of
> that.

If we were going to do it like that, I'd vote for

--with-uuid={ossp,e2fs,bsd}

with no default at present (ie you can't say just "--with-uuid",
though we'd have the option to allow that in future). But I doubt
this is better than the --with-foo-uuid spelling.

regards, tom lane

On May 27, 2014, at 7:44 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> In either case, the problem remains of exactly what to call the
> e2fsprogs-derived implementation. It does seem that people who are
> familiar with these libraries call it that, but I'm worried that such
> a name will confuse those not so familiar.

--with-libuuid?

D

On 27 May 2014 18:33:48 EEST, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> Tom Lane wrote:
>>> The problem is that the long-established spelling is
>--with-ossp-uuid.
>>> I don't think we can break that case. While we could set up
>something
>>> like what you propose alongside it, it doesn't seem like there's any
>>> advantage to doing so compared to inventing --with-foo-uuid as
>needed.
>
>> I was thinking that --with-ossp-uuid would still be required to
>enable
>> UUID generators at all; the other one just selects the implementation
>to
>> use, which defaults to OSSP to maintain backwards compatibility.
>Maybe
>> introduce --with-uuid and have --with-ossp-uuid a deprecated synonym
>of
>> that.
>
>If we were going to do it like that, I'd vote for
>
> --with-uuid={ossp,e2fs,bsd}
>
>with no default at present (ie you can't say just "--with-uuid",
>though we'd have the option to allow that in future). But I doubt
>this is better than the --with-foo-uuid spelling.

FWIW, --with-uuid=foo looks much clearer to me.

- Heikki

Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
> On 27 May 2014 18:33:48 EEST, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> If we were going to do it like that, I'd vote for
>>
>> --with-uuid={ossp,e2fs,bsd}
>>
>> with no default at present (ie you can't say just "--with-uuid",
>> though we'd have the option to allow that in future). But I doubt
>> this is better than the --with-foo-uuid spelling.

> FWIW, --with-uuid=foo looks much clearer to me.

OK, I'll make it so.

regards, tom lane

Pushed; thanks for working on this!

regards, tom lane

On 5/26/14, 1:25 PM, Tom Lane wrote:
> Assuming this works as advertised, does anyone have an objection to
> pushing it into 9.4?

Yes, it's way too late for that.

On 5/27/14, 2:41 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnakangas(at)vmware(dot)com> writes:
>> On 27 May 2014 18:33:48 EEST, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> If we were going to do it like that, I'd vote for
>>>
>>> --with-uuid={ossp,e2fs,bsd}
>>>
>>> with no default at present (ie you can't say just "--with-uuid",
>>> though we'd have the option to allow that in future). But I doubt
>>> this is better than the --with-foo-uuid spelling.
>
>> FWIW, --with-uuid=foo looks much clearer to me.
>
> OK, I'll make it so.

I'm shocked that this new feature has been committed post beta with less
than 48 hours of review time over a holiday weekend. This issue has
been known for years. Why does it need to be solved right now?

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> I'm shocked that this new feature has been committed post beta with less
> than 48 hours of review time over a holiday weekend. This issue has
> been known for years. Why does it need to be solved right now?

As per the commit message: our packagers were screaming about the fact
that they couldn't build uuid-ossp anymore now that autoconf 2.69 has
tightened up its validity checks for headers. If you don't like this
change, we can revert it and also revert the upgrade to 2.69.

I'm not terribly happy about pushing such a change post-beta1 either,
but it's not like this isn't something we've known was needed. Anyway,
what's the worst case if we find a bug here? Tell people not to use
uuid-ossp?

regards, tom lane

On 5/27/14, 10:37 PM, Tom Lane wrote:
> I'm not terribly happy about pushing such a change post-beta1 either,
> but it's not like this isn't something we've known was needed. Anyway,
> what's the worst case if we find a bug here? Tell people not to use
> uuid-ossp?

Mainly some more discussion time would have been nice. Also, while the
old ossp-based uuid was broken in some cases, it had a well-defined
workaround. The new code introduces a whole new dimension of potential
portability issues.

On 5/27/14, 10:37 PM, Tom Lane wrote:
> If you don't like this change, we can revert it and also revert the upgrade to 2.69.

Nobody else appears to be concerned, but I would have preferred this option.

On 2014-05-29 08:14:48 -0400, Peter Eisentraut wrote:
> On 5/27/14, 10:37 PM, Tom Lane wrote:
> > If you don't like this change, we can revert it and also revert the upgrade to 2.69.
>
> Nobody else appears to be concerned, but I would have preferred this option.

I am pretty concerned actually. But I don't see downgrading to an
earlier autoconf as something really helpful. There already was a huge
portability problem with the current code. Avoiding the autoconf update
for a while wouldn't have solved it. And in 5 years time the amount of
portability problems will be much larger.
Yes, it'd have been nice if this were done a month+ ago. But nobody
stepped up :(. Seems like the least bad choice :/

Greetings,

Andres Freund

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

On 05/29/2014 08:21 AM, Andres Freund wrote:
> On 2014-05-29 08:14:48 -0400, Peter Eisentraut wrote:
>> On 5/27/14, 10:37 PM, Tom Lane wrote:
>>> If you don't like this change, we can revert it and also revert the upgrade to 2.69.
>> Nobody else appears to be concerned, but I would have preferred this option.
> I am pretty concerned actually. But I don't see downgrading to an
> earlier autoconf as something really helpful. There already was a huge
> portability problem with the current code. Avoiding the autoconf update
> for a while wouldn't have solved it. And in 5 years time the amount of
> portability problems will be much larger.
> Yes, it'd have been nice if this were done a month+ ago. But nobody
> stepped up :(. Seems like the least bad choice :/
>

The most worrying thing is that we didn't find the occasioning problem
when we switched to autoconf 2.69 back in December, so we end up dealing
with bad choices now.

cheers

andrew

On 5/29/14, 8:21 AM, Andres Freund wrote:
> But I don't see downgrading to an
> earlier autoconf as something really helpful.

Well, we could have just hacked up that particular header check to do
what we want.

On 2014-05-29 08:49:38 -0400, Peter Eisentraut wrote:
> On 5/29/14, 8:21 AM, Andres Freund wrote:
> > But I don't see downgrading to an
> > earlier autoconf as something really helpful.
>
> Well, we could have just hacked up that particular header check to do
> what we want.

Still wouldn't have solved that ossp already didn't work on several
platforms at all anymore and is likely to work on even fewer in 5 years.

Greetings,

Andres Freund

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

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-29 08:49:38 -0400, Peter Eisentraut wrote:
>> Well, we could have just hacked up that particular header check to do
>> what we want.

> Still wouldn't have solved that ossp already didn't work on several
> platforms at all anymore and is likely to work on even fewer in 5 years.

Yeah. The problem is not with the header check, it's with the fact that
OSSP UUID is basically broken on several modern platforms.[1] We were
going to have to do something about that pretty soon anyway. I agree that
this isn't the most ideal time in the dev cycle to do something about it,
but fixing portability issues is one of the expected beta-time activities,
no? That's really what this is.

regards, tom lane

[1] Quite aside from compilation problems, yesterday's testing results
suggest that it can't read the system MAC address at all on Debian:
http://www.postgresql.org/message-id/29063.1401333825@sss.pgh.pa.us

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 05/29/2014 08:21 AM, Andres Freund wrote:
>> Yes, it'd have been nice if this were done a month+ ago. But nobody
>> stepped up :(. Seems like the least bad choice :/

> The most worrying thing is that we didn't find the occasioning problem
> when we switched to autoconf 2.69 back in December, so we end up dealing
> with bad choices now.

I think the main reason for that is that none of the buildfarm animals
were trying to build contrib/uuid-ossp on machines where OSSP UUID is
shaky. Some of our packagers do so, though, and so once the beta got
into their hands we found out about the problem.

Short of adding "try to package according to Debian, Red Hat, etc
packaging recipes" to the buildfarm requirements, there doesn't seem to
be much that we could do to find out about such issues earlier. (And
no, I'm not proposing that as a good idea. As an ex-packager, I know
that those recipes are moving targets because of constantly changing
external requirements. We don't want to take over that maintenance.)

So IMO we just have to expect that beta is going to turn up portability
issues, and we're going to have to do what it takes to resolve them,
even if it's nontrivial.

The good news on this front is that we have substantially more buildfarm
coverage of contrib/uuid-ossp than we had two days ago.

regards, tom lane

On Thu, May 29, 2014 at 02:21:57PM +0200, Andres Freund wrote:
> On 2014-05-29 08:14:48 -0400, Peter Eisentraut wrote:
> > On 5/27/14, 10:37 PM, Tom Lane wrote:
> > > If you don't like this change, we can revert it and also revert the upgrade to 2.69.
> >
> > Nobody else appears to be concerned, but I would have preferred this option.
>
> I am pretty concerned actually. But I don't see downgrading to an
> earlier autoconf as something really helpful. There already was a huge
> portability problem with the current code. Avoiding the autoconf update
> for a while wouldn't have solved it. And in 5 years time the amount of
> portability problems will be much larger.
> Yes, it'd have been nice if this were done a month+ ago. But nobody
> stepped up :(. Seems like the least bad choice :/

One thing that concerns me is that we already had the problem that users
creating the uuid-ossp extension had to double-quote the name because of
the dash, and we have regularly questioned the viability of the
uuid-ossp codebase.

Now that we know we have to support alternatives, we are changing the
build API to support those alternatives, but doing nothing to decouple
the extension name from uuid-ossp and the dash issue.

Seems this would be the logical time to just break compatibility and get
a sane API for UUID generation. We could hack pg_dump --binary-upgrade
to map a dump of "uuid-ossp" to some other name, or hack the backend to
accept "uuid-ossp" and map that to something more sane.

I basically feel this change is making our bad API even more confusing.
I think the only good thing about this change is that the _user_ API, as
odd as it is, remains the same.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> One thing that concerns me is that we already had the problem that users
> creating the uuid-ossp extension had to double-quote the name because of
> the dash, and we have regularly questioned the viability of the
> uuid-ossp codebase.

> Now that we know we have to support alternatives, we are changing the
> build API to support those alternatives, but doing nothing to decouple
> the extension name from uuid-ossp and the dash issue.

Well, if you've got a proposal for how to rename the extension without
creating major compatibility problems, let's hear it.

> Seems this would be the logical time to just break compatibility and get
> a sane API for UUID generation.

Most people think the "sane API" would be to put the functionality in
core, and forget about any extension name at all. The compatibility
problems with that approach aren't exactly trivial either, but I suspect
that's where we'll end up in the long run. So I'm not that excited about
kluge solutions for renaming the extension.

regards, tom lane

On Thu, May 29, 2014 at 01:56:22PM -0400, Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > One thing that concerns me is that we already had the problem that users
> > creating the uuid-ossp extension had to double-quote the name because of
> > the dash, and we have regularly questioned the viability of the
> > uuid-ossp codebase.
>
> > Now that we know we have to support alternatives, we are changing the
> > build API to support those alternatives, but doing nothing to decouple
> > the extension name from uuid-ossp and the dash issue.
>
> Well, if you've got a proposal for how to rename the extension without
> creating major compatibility problems, let's hear it.

Well, the only two I could think of were the pg_dump and backend mapping
changes; pretty hacky.

> > Seems this would be the logical time to just break compatibility and get
> > a sane API for UUID generation.
>
> Most people think the "sane API" would be to put the functionality in
> core, and forget about any extension name at all. The compatibility
> problems with that approach aren't exactly trivial either, but I suspect
> that's where we'll end up in the long run. So I'm not that excited about
> kluge solutions for renaming the extension.

OK. I was just worried that users are now using a badly-named uuid-ossp
extension that isn't even using uuid-ossp in many cases. If we have a
long-term plan to fix this, then you are right that it isn't worth
worrying about it now.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On Thu, May 29, 2014 at 1:56 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
>> One thing that concerns me is that we already had the problem that users
>> creating the uuid-ossp extension had to double-quote the name because of
>> the dash, and we have regularly questioned the viability of the
>> uuid-ossp codebase.
>
>> Now that we know we have to support alternatives, we are changing the
>> build API to support those alternatives, but doing nothing to decouple
>> the extension name from uuid-ossp and the dash issue.
>
> Well, if you've got a proposal for how to rename the extension without
> creating major compatibility problems, let's hear it.
>
>> Seems this would be the logical time to just break compatibility and get
>> a sane API for UUID generation.
>
> Most people think the "sane API" would be to put the functionality in
> core, and forget about any extension name at all. The compatibility
> problems with that approach aren't exactly trivial either, but I suspect
> that's where we'll end up in the long run. So I'm not that excited about
> kluge solutions for renaming the extension.

FWIW, I'm with Bruce. I see no compelling reason to put this
functionality in core, but it sure would be nice to have it named
something less dumb. (Don't ask me how to get there, because I
dunno.)

Also FWIW, I agree that adding this portability code now was the right
decision. I can't see that this introduced significant instability
into PostgreSQL; if anything, it probably decreased it, because before
we had each packager hacking on it on their own, and coming up with
their own ad-hoc solutions. Now we have an official way of doing this
which we can at least hope will be universally adopted. Perhaps Tom
could have waited a bit longer before committing to make sure all
voices were heard, but had discussion ensued I would have voted
strongly for the change.

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

On Tue, May 27, 2014 at 07:46:41PM -0400, Tom Lane wrote:
> Pushed; thanks for working on this!

Here's a fix to make the MSVC build process account for the addition of
HAVE_UUID_OSSP. (None of the MSVC buildfarm members enable uuid-ossp.)

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Noah Misch <noah(at)leadboat(dot)com> writes:
> Here's a fix to make the MSVC build process account for the addition of
> HAVE_UUID_OSSP. (None of the MSVC buildfarm members enable uuid-ossp.)

Looks reasonable. I'm unable to test this scenario, but if you have,
please commit.

regards, tom lane