Обсуждение: [PATCH] Implement uuid_version()

Hackers,

     While working on an application, the need arose to be able 
efficiently differentiate v4/v5 UUIDs (for use in partial indexes, among 
others)

... so please find attached a trivial patch which adds the 
functionality. The "uuid_version_bits()" function (from the test suite?) 
seems quite a bit hackish, apart from inefficient :(


     I'm not sure whether this actually would justify a version bump for 
the OSSP-UUID extension ---a misnomer, BTW, since at least in all the 
systems I have access to, the extension is actually linked against 
libuuid from e2fsutils, but I digress --- or not, given that it doesn't 
change exposed functionality.


     Another matter, which I'd like to propose in a later thread, is 
whether it'd be interesting to include the main UUID functionality 
directly in core, with the remaining functions in ossp-uuid (just like 
it is now, for backwards compatibility): Most current patterns for 
distributed/sharded databases are based on using UUIDs for many PKs.


Thanks,

     J.L.

Jose Luis Tallon <jltallon@adv-solutions.net> writes:
>      While working on an application, the need arose to be able 
> efficiently differentiate v4/v5 UUIDs (for use in partial indexes, among 
> others)
> ... so please find attached a trivial patch which adds the 
> functionality.

No particular objection...

>      I'm not sure whether this actually would justify a version bump for 
> the OSSP-UUID extension

Yes.  Basically, once we've shipped a given version of an extension's
SQL script, that version is *frozen*.  Anything at all that you want
to do to it has to be done in an extension update script, because
otherwise there's no clean migration path for users.

So basically, leave uuid-ossp--1.1.sql as it stands, and put the
new CREATE FUNCTION in a new uuid-ossp--1.1--1.2.sql script.
See any recent patch that updated an extension for an example, eg
commit eb6f29141bed9dc95cb473614c30f470ef980705.

(We do allow exceptions when somebody's already updated the extension
in the current devel cycle, but that doesn't apply here.)

> Another matter, which I'd like to propose in a later thread, is 
> whether it'd be interesting to include the main UUID functionality 
> directly in core

We've rejected that before, and I don't see any reason to think
the situation has changed since prior discussions.

            regards, tom lane

On 6/4/19 18:35, Tom Lane wrote:
> Jose Luis Tallon <jltallon@adv-solutions.net> writes:
>>       While working on an application, the need arose to be able
>> efficiently differentiate v4/v5 UUIDs (for use in partial indexes, among
>> others)
>> ... so please find attached a trivial patch which adds the
>> functionality.
> No particular objection...
>
>>       I'm not sure whether this actually would justify a version bump for
>> the OSSP-UUID extension
> Yes.  Basically, once we've shipped a given version of an extension's
> SQL script, that version is *frozen*.  Anything at all that you want
> to do to it has to be done in an extension update script, because
> otherwise there's no clean migration path for users.

Got it, and done. Please find attached a v2 patch with the upgrade 
script included.


Thank you for taking a look. Your time is much appreciated :)


     J.L.

On Sat, Apr 06, 2019 at 12:35:47PM -0400, Tom Lane wrote:
> Jose Luis Tallon <jltallon@adv-solutions.net> writes:
> >      While working on an application, the need arose to be able 
> > efficiently differentiate v4/v5 UUIDs (for use in partial indexes, among 
> > others)
> > ... so please find attached a trivial patch which adds the 
> > functionality.
> 
> No particular objection...
> 
> >      I'm not sure whether this actually would justify a version bump for 
> > the OSSP-UUID extension
> 
> Yes.  Basically, once we've shipped a given version of an extension's
> SQL script, that version is *frozen*.  Anything at all that you want
> to do to it has to be done in an extension update script, because
> otherwise there's no clean migration path for users.
> 
> So basically, leave uuid-ossp--1.1.sql as it stands, and put the
> new CREATE FUNCTION in a new uuid-ossp--1.1--1.2.sql script.
> See any recent patch that updated an extension for an example, eg
> commit eb6f29141bed9dc95cb473614c30f470ef980705.
> 
> (We do allow exceptions when somebody's already updated the extension
> in the current devel cycle, but that doesn't apply here.)
> 
> > Another matter, which I'd like to propose in a later thread, is 
> > whether it'd be interesting to include the main UUID functionality 
> > directly in core
> 
> We've rejected that before, and I don't see any reason to think
> the situation has changed since prior discussions.

I see some.

UUIDs turn out to be super useful in distributed systems to give good
guarantees of uniqueness without coordinating with a particular node.
Such systems have become a good bit more common since the most recent
time this was discussed.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Apr 7, 2019 at 10:15 AM David Fetter <david@fetter.org> wrote:
> I see some.
>
> UUIDs turn out to be super useful in distributed systems to give good
> guarantees of uniqueness without coordinating with a particular node.
> Such systems have become a good bit more common since the most recent
> time this was discussed.

That's not really a compelling reason, though, because anybody who
needs UUIDs can always install the extension.  And on the other hand,
if we moved UUID support into core, then we'd be adding a hard compile
dependency on one of the UUID facilities, which might annoy some
developers.  We could possibly work around that by implementing our
own UUID facilities in core, but I'm not volunteering to do the work,
and I'm not sure that the work has enough benefit to justify the
labor.

My biggest gripe about uuid-ossp is that the name is stupid.  I wish
we could see our way clear to renaming that extension to just 'uuid',
because as J.L. says, virtually nobody's actually compiling against
the OSSP library any more.  The trick there is how to do that without
annoying exiting users.  Maybe we could leave behind an "upgrade"
script for the uuid-ossp extension that does CREATE EXTENSION uuid,
then alters all objects owned by the current extension to be owned by
the new extension, and maybe even drops itself.

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

Robert Haas <robertmhaas@gmail.com> writes:
> My biggest gripe about uuid-ossp is that the name is stupid.  I wish
> we could see our way clear to renaming that extension to just 'uuid',
> because as J.L. says, virtually nobody's actually compiling against
> the OSSP library any more.

+1

There's no ALTER EXTENSION RENAME, and I suppose there can't be because
it would require editing/rewriting on-disk files that the server might
not even have write permissions for.  But your idea of an "update"
script that effectively moves everything over into a new extension
(that's physically installed but not present in current database)
might work.

Another way to approach it would be to have a script that belongs
to the new extension and what you do is
    CREATE EXTENSION uuid FROM "uuid_ossp";
to perform the migration of the SQL objects.

Either way, we'd be looking at providing two .so's for some period
of time, but fortunately they're small.

            regards, tom lane

On 8/4/19 17:06, Robert Haas wrote:
> On Sun, Apr 7, 2019 at 10:15 AM David Fetter <david@fetter.org> wrote:
>> I see some.
>>
>> UUIDs turn out to be super useful in distributed systems to give good
>> guarantees of uniqueness without coordinating with a particular node.
>> Such systems have become a good bit more common since the most recent
>> time this was discussed.
> That's not really a compelling reason, though, because anybody who
> needs UUIDs can always install the extension.  And on the other hand,
> if we moved UUID support into core, then we'd be adding a hard compile
> dependency on one of the UUID facilities, which might annoy some
> developers.  We could possibly work around that by implementing our
> own UUID facilities in core,

Yup. My proposal basically revolves around implementing v3 / v4 / v5 
(most used/useful versions for the aforementioned use cases) in core, 
using the already existing md5 and sha1 facilities (which are already 
being linked from the current uuid-ossp extension as fallback with 
certain configurations) ... and leaving the remaining functionality in 
the extension, just as it is now.

This way, we guarantee backwards compatibility: Those already using the 
extension wouldn't have to change anything, and new users won't need to 
load any extension to benefit from this (base) functionality.

>   but I'm not volunteering to do the work,
Of course, I'd take care of that :)
> and I'm not sure that the work has enough benefit to justify the
> labor.

With this "encouragement", I'll write the code and submit the patches to 
a future commitfest. Then the normal procedure will take care of judging 
whether it's worth being included or not :$

> My biggest gripe about uuid-ossp is that the name is stupid.  I wish
> we could see our way clear to renaming that extension to just 'uuid',
> because as J.L. says, virtually nobody's actually compiling against
> the OSSP library any more.  The trick there is how to do that without
> annoying exiting users.  Maybe we could leave behind an "upgrade"
> script for the uuid-ossp extension that does CREATE EXTENSION uuid,
> then alters all objects owned by the current extension to be owned by
> the new extension, and maybe even drops itself.

I believe my proposal above mostly solves the issue: new users with 
"standard" needs won't need to load any extension (better than current), 
old users will get the same functionality as they have today (only part 
in core and part in the extension)...

  ...and a relatively simple "alias" (think Linux kernel modules) 
facility would make the transition fully transparent: rename extension 
to "uuid" ---possibly dropping the dependency on uuid-ossp in the 
process--- and expose an "uuid-ossp" alias for backwards compatibility.


Thanks,

     J.L.

Hi,

On 2019-04-08 11:06:57 -0400, Robert Haas wrote:
> That's not really a compelling reason, though, because anybody who
> needs UUIDs can always install the extension.  And on the other hand,
> if we moved UUID support into core, then we'd be adding a hard compile
> dependency on one of the UUID facilities, which might annoy some
> developers.  We could possibly work around that by implementing our
> own UUID facilities in core, but I'm not volunteering to do the work,
> and I'm not sure that the work has enough benefit to justify the
> labor.

The randomness based UUID generators don't really have dependencies, now
that we have a dependency on strong randomness.  I kinda thing the
dependency argument actually works *against* uuid-ossp - precisely
because of its dependencies (which also vary by OS) it's not a proper
replacement for a type of facility a very sizable fraction of our users
need.

Greetings,

Andres Freund

On 2019-04-08 23:06, Andres Freund wrote:
> The randomness based UUID generators don't really have dependencies, now
> that we have a dependency on strong randomness.  I kinda thing the
> dependency argument actually works *against* uuid-ossp - precisely
> because of its dependencies (which also vary by OS) it's not a proper
> replacement for a type of facility a very sizable fraction of our users
> need.

Yeah, I think implementing a v4 generator in core would be trivial and
address almost everyone's requirements.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-04-09 08:04, Peter Eisentraut wrote:
> On 2019-04-08 23:06, Andres Freund wrote:
>> The randomness based UUID generators don't really have dependencies, now
>> that we have a dependency on strong randomness.  I kinda thing the
>> dependency argument actually works *against* uuid-ossp - precisely
>> because of its dependencies (which also vary by OS) it's not a proper
>> replacement for a type of facility a very sizable fraction of our users
>> need.
> 
> Yeah, I think implementing a v4 generator in core would be trivial and
> address almost everyone's requirements.

Here is a proposed patch for this.  I did a fair bit of looking around
in other systems for a naming pattern but didn't find anything
consistent.  So I ended up just taking the function name and code from
pgcrypto.

As you can see, the code is trivial and has no external dependencies.  I
think this would significantly upgrade the usability of the uuid type.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/6/19 10:49, Peter Eisentraut wrote:
> On 2019-04-09 08:04, Peter Eisentraut wrote:
>> On 2019-04-08 23:06, Andres Freund wrote:
>>> The randomness based UUID generators don't really have dependencies, now
>>> that we have a dependency on strong randomness.  I kinda thing the
>>> dependency argument actually works *against* uuid-ossp - precisely
>>> because of its dependencies (which also vary by OS) it's not a proper
>>> replacement for a type of facility a very sizable fraction of our users
>>> need.
>> Yeah, I think implementing a v4 generator in core would be trivial and
>> address almost everyone's requirements.
> Here is a proposed patch for this.  I did a fair bit of looking around
> in other systems for a naming pattern but didn't find anything
> consistent.  So I ended up just taking the function name and code from
> pgcrypto.
>
> As you can see, the code is trivial and has no external dependencies.  I
> think this would significantly upgrade the usability of the uuid type.

Yes, indeed. Thanks!

This is definitively a good step towards removing external dependencies 
for general usage of UUIDs. As recently commented, enabling extensions 
at some MSPs/Cloud providers can be a bit challenging.


I wonder whether re-implementing some more of the extension's (ie. UUID 
v5) in terms of PgCrypto and in-core makes sense / would actually be 
accepted into core?

I assume that Peter would like to commit that potential patch series?


Thanks,

     / J.L.

On 2019-06-11 12:31, Jose Luis Tallon wrote:
> I wonder whether re-implementing some more of the extension's (ie. UUID 
> v5) in terms of PgCrypto and in-core makes sense / would actually be 
> accepted into core?

Those other versions are significantly more complicated to implement,
and I don't think many people really need them, so I'm not currently
interested.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/6/19 13:11, Peter Eisentraut wrote:
> On 2019-06-11 12:31, Jose Luis Tallon wrote:
>> I wonder whether re-implementing some more of the extension's (ie. UUID
>> v5) in terms of PgCrypto and in-core makes sense / would actually be
>> accepted into core?
> Those other versions are significantly more complicated to implement,
> and I don't think many people really need them, so I'm not currently
> interested.

For the record: I was volunteering to implement that functionality. I'd 
only need some committer to take a look and erm... commit it :)

Thank you, in any case; The patch you have provided will be very useful.


     / J.L.

On Mon, Apr 8, 2019 at 11:04 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> Yeah, I think implementing a v4 generator in core would be trivial and
> address almost everyone's requirements.

FWIW, I think that we could do better with nbtree page splits given
sequential UUIDs of one form or another [1]. We could teach
nbtsplitloc.c to pack leaf pages full of UUIDs in the event of the
user using sequential UUIDs. With a circular UUID prefix, I think
you'll run into an issue similar to the issue that was addressed by
the "split after new tuple" optimization -- most leaf pages end up 50%
full. I've not verified this, but I can't see why it would be any
different to other multimodal key space with sequential insertions
that are grouped. Detecting this in UUIDs may or may not require
opclass infrastructure. Either way, I'm not likely to work on it until
there is a clear target, such as a core or contrib sequential UUID
generator. Though I am looking at various ways to improve
nbtsplitloc.c for Postgres 13 -- I suspect that additional wins are
possible.

Any sequential UUID scheme will already have far fewer problems with
indexing today, since random UUIDs are *dreadful*, but I can imagine
doing quite a lot better still. Application developers love UUIDs. We
should try to meet them where they are.

[1] https://www.2ndquadrant.com/en/blog/sequential-uuid-generators/
-- 
Peter Geoghegan

Hello Peter,

>> Yeah, I think implementing a v4 generator in core would be trivial and
>> address almost everyone's requirements.
>
> Here is a proposed patch for this.  I did a fair bit of looking around
> in other systems for a naming pattern but didn't find anything
> consistent.  So I ended up just taking the function name and code from
> pgcrypto.
>
> As you can see, the code is trivial and has no external dependencies.  I
> think this would significantly upgrade the usability of the uuid type.

Patch applies cleanly.

However it does not compile, it fails on: "Duplicate OIDs detected: 3429".

Someone inserted a new entry since it was produced.

I'm wondering whether pg_random_uuid() should be taken out of pgcrypto if 
it is available in core?

-- 
Fabien.

On Fri, Jun 28, 2019 at 03:24:03PM -0700, Peter Geoghegan wrote:
>On Mon, Apr 8, 2019 at 11:04 PM Peter Eisentraut
><peter.eisentraut@2ndquadrant.com> wrote:
>> Yeah, I think implementing a v4 generator in core would be trivial and
>> address almost everyone's requirements.
>
>FWIW, I think that we could do better with nbtree page splits given
>sequential UUIDs of one form or another [1]. We could teach
>nbtsplitloc.c to pack leaf pages full of UUIDs in the event of the
>user using sequential UUIDs. With a circular UUID prefix, I think
>you'll run into an issue similar to the issue that was addressed by
>the "split after new tuple" optimization -- most leaf pages end up 50%
>full. I've not verified this, but I can't see why it would be any
>different to other multimodal key space with sequential insertions
>that are grouped.

I think the state with pages being only 50% full is only temporary,
because thanks to the prefix being circular we'll get back to the page
eventually and add more tuples to it.

It's not quite why I made the prefix circular (in my extension) - that was
to allow reuse of space after deleting rows. But I think it should help
with this too.


> Detecting this in UUIDs may or may not require
>opclass infrastructure. Either way, I'm not likely to work on it until
>there is a clear target, such as a core or contrib sequential UUID
>generator. Though I am looking at various ways to improve nbtsplitloc.c
>for Postgres 13 -- I suspect that additional wins are possible.
>

I'm not against improving this, although I don't have a very clear idea
how it should work in the end. But UUIDs are used pretty commonly so it's
a worthwhile optimization area.

>Any sequential UUID scheme will already have far fewer problems with
>indexing today, since random UUIDs are *dreadful*, but I can imagine
>doing quite a lot better still. Application developers love UUIDs. We
>should try to meet them where they are.
>

I agree.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-06-30 14:50, Fabien COELHO wrote:
> I'm wondering whether pg_random_uuid() should be taken out of pgcrypto if 
> it is available in core?

That would probably require an extension version update dance in
pgcrypto.  I'm not sure if it's worth that.  Thoughts?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2/7/19 9:26, Peter Eisentraut wrote:
> On 2019-06-30 14:50, Fabien COELHO wrote:
>> I'm wondering whether pg_random_uuid() should be taken out of pgcrypto if
>> it is available in core?
> That would probably require an extension version update dance in
> pgcrypto.  I'm not sure if it's worth that.  Thoughts?

What I have devised for my upcoming patch series is to use a 
compatibility "shim" that calls the corresponding core code when the 
expected usage does not match the new names/signatures...

This way we wouldn't even need to version bump pgcrypto (full backwards 
compatibility -> no bump needed). Another matter is whether this should 
raise some "deprecation warning" or the like; I don't think we have any 
such mechanisms available yet.


FWIW, I'm implementing an "alias" functionality for extensions, too, in 
order to achieve transparent (for the user) extension renames.

HTH


Thanks,

     / J.L.

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 2019-06-30 14:50, Fabien COELHO wrote:
>> I'm wondering whether pg_random_uuid() should be taken out of pgcrypto if 
>> it is available in core?

> That would probably require an extension version update dance in
> pgcrypto.  I'm not sure if it's worth that.  Thoughts?

We have some previous experience with this type of thing when we migrated
contrib/tsearch2 stuff into core.  I'm too caffeine-deprived to remember
exactly what we did or how well it worked.  But it seems advisable to go
study that history, because we could easily make things a mess for users
if we fail to consider their upgrade experience.

            regards, tom lane

On 2019-07-02 17:09, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 2019-06-30 14:50, Fabien COELHO wrote:
>>> I'm wondering whether pg_random_uuid() should be taken out of pgcrypto if 
>>> it is available in core?
> 
>> That would probably require an extension version update dance in
>> pgcrypto.  I'm not sure if it's worth that.  Thoughts?
> 
> We have some previous experience with this type of thing when we migrated
> contrib/tsearch2 stuff into core.  I'm too caffeine-deprived to remember
> exactly what we did or how well it worked.  But it seems advisable to go
> study that history, because we could easily make things a mess for users
> if we fail to consider their upgrade experience.

I think in that case we wanted users of the extension to transparently
end up using the in-core code.  This is not the case here: Both the
extension and the proposed in-core code do the same thing and there is
very little code duplication, so having them coexist would be fine in
principle.

I think the alternatives are:

1. We keep the code in both places.  This is fine.  There is no problem
with having the same C function or the same SQL function name in both
places.

2. We remove the C function from pgcrypto and make an extension version
bump.  This will create breakage for (some) current users of the
function from pgcrypto.

So option 2 would ironically punish the very users we are trying to
help.  So I think just doing nothing is the best option.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> I think the alternatives are:

> 1. We keep the code in both places.  This is fine.  There is no problem
> with having the same C function or the same SQL function name in both
> places.

> 2. We remove the C function from pgcrypto and make an extension version
> bump.  This will create breakage for (some) current users of the
> function from pgcrypto.

> So option 2 would ironically punish the very users we are trying to
> help.  So I think just doing nothing is the best option.

Hm.  Option 1 means that it's a bit unclear which function you are
actually calling.  As long as the implementations behave identically,
that seems okay, but I wonder if that's a constraint we want for the
long term.

A possible option 3 is to keep the function in pgcrypto but change
its C code to call the core code.

            regards, tom lane

On 2019-Jul-04, Tom Lane wrote:

> A possible option 3 is to keep the function in pgcrypto but change
> its C code to call the core code.

This seems most reasonable, and is what José Luis proposed upthread.  We
don't have to bump the pgcrypto extension version, as nothing changes
for pgcrypto externally.

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

On 4/7/19 17:30, Alvaro Herrera wrote:
> On 2019-Jul-04, Tom Lane wrote:
>
>> A possible option 3 is to keep the function in pgcrypto but change
>> its C code to call the core code.
> This seems most reasonable, and is what José Luis proposed upthread.  We
> don't have to bump the pgcrypto extension version, as nothing changes
> for pgcrypto externally.

Yes, indeed.

...which means I get another todo item if nobody else volunteers :)


Thanks!

     / J.L.

On 2019-07-05 00:08, Jose Luis Tallon wrote:
> On 4/7/19 17:30, Alvaro Herrera wrote:
>> On 2019-Jul-04, Tom Lane wrote:
>>
>>> A possible option 3 is to keep the function in pgcrypto but change
>>> its C code to call the core code.

Updated patch with this change included.

(There is also precedent for redirecting the extension function to the
internal one by changing the SQL-level function definition using CREATE
OR REPLACE FUNCTION ... LANGUAGE INTERNAL.  But that seems more
complicated and would require a new extension version.  It could maybe
be included if the extension version is changed for other reasons.)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/7/19 11:00, Peter Eisentraut wrote:
> On 2019-07-05 00:08, Jose Luis Tallon wrote:
>> On 4/7/19 17:30, Alvaro Herrera wrote:
>>> On 2019-Jul-04, Tom Lane wrote:
>>>
>>>> A possible option 3 is to keep the function in pgcrypto but change
>>>> its C code to call the core code.
> Updated patch with this change included.
Great, thanks!

On 2019-Jul-05, Peter Eisentraut wrote:

> (There is also precedent for redirecting the extension function to the
> internal one by changing the SQL-level function definition using CREATE
> OR REPLACE FUNCTION ... LANGUAGE INTERNAL.  But that seems more
> complicated and would require a new extension version.

One issue with this approach is that it forces the internal function to
remain unchanged forever.  That seems OK in this particular case.

> It could maybe be included if the extension version is changed for
> other reasons.)

Maybe add a comment in the control file (?) so that we remember to do it
then.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Jul-05, Peter Eisentraut wrote:
>> (There is also precedent for redirecting the extension function to the
>> internal one by changing the SQL-level function definition using CREATE
>> OR REPLACE FUNCTION ... LANGUAGE INTERNAL.  But that seems more
>> complicated and would require a new extension version.

> One issue with this approach is that it forces the internal function to
> remain unchanged forever.  That seems OK in this particular case.

No, what it's establishing is that the extension and core functions
will do the same thing forevermore.  Seems to me that's what we want
here.

>> It could maybe be included if the extension version is changed for
>> other reasons.)

> Maybe add a comment in the control file (?) so that we remember to do it
> then.

I'm not terribly excited about that --- we'd still need to keep the
C function redirection in place in the .so file, for benefit of
people who hadn't done ALTER EXTENSION UPGRADE.

            regards, tom lane

Hello Peter,

>>>> A possible option 3 is to keep the function in pgcrypto but change
>>>> its C code to call the core code.
>
> Updated patch with this change included.

Patch applies cleanly, compiles (both pg and pgcrypto). make check (global 
and pgcrypto) ok. Doc generation ok. Minor comments:

About doc: I'd consider "generation" instead of "generating" as a 
secondary index term.

> (There is also precedent for redirecting the extension function to the
> internal one by changing the SQL-level function definition using CREATE
> OR REPLACE FUNCTION ... LANGUAGE INTERNAL.  But that seems more
> complicated and would require a new extension version.  It could maybe
> be included if the extension version is changed for other reasons.)

What about avoiding a redirection with something like:

Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid;

-- 
Fabien.

Hello Jose,

> Got it, and done. Please find attached a v2 patch with the upgrade script 
> included.

Patch v2 applies cleanly. Compiles cleanly (once running configure 
--with-uuid=...). Local make check ok. Doc build ok.

There are no tests, I'd suggest to add some under sql & change expected if 
possible which would return all possible values, including with calling 
pg_random_uuid() which should return 4.

Documentation describes uuid_version(), should it not describe 
uuid_version(namespace uuid)?

I'd suggest to add an example.

The extension update script seems ok, but ISTM that "uuid-ossp-1.1.sql" 
should be replaced with an updated "uuid-ossp-1.2.sql".

-- 
Fabien.

On 13/7/19 8:31, Fabien COELHO wrote:
>
> Hello Jose,

Hello, Fabien

Thanks for taking a look

>
>> Got it, and done. Please find attached a v2 patch with the upgrade 
>> script included.
>
> Patch v2 applies cleanly. Compiles cleanly (once running configure 
> --with-uuid=...). Local make check ok. Doc build ok.
>
> There are no tests, I'd suggest to add some under sql & change 
> expected if possible which would return all possible values, including 
> with calling pg_random_uuid() which should return 4.
>
> Documentation describes uuid_version(), should it not describe 
> uuid_version(namespace uuid)?
>
> I'd suggest to add an example.
>
> The extension update script seems ok, but ISTM that 
> "uuid-ossp-1.1.sql" should be replaced with an updated 
> "uuid-ossp-1.2.sql".
>
This was a quite naïf approach to the issue on my part, more a "scratch 
my own itch" than anything else.... but definitively sparked some 
interest. Thanks to all involved.

Considering the later arguments on-list, I plan on submitting a more 
elaborate patchset integrating the feedback received so far, and along 
the following lines:

- uuid type, v4 generation and uuid_version() in core

- Provide a means to rename/supercede extensions keeping backwards 
compatibility (i.e. uuid-ossp -> uuid, keep old code working)

- Miscellaneous other functionality

- Drop "dead" code

   ...but I've tried to keep quiet so as not to disturb too much around 
release time.


I intend to continue working on this in late July, aiming for the 
following commitfest (once more "urgent" patches will have landed)


Thanks again.

     J.L.

On 2019-07-13 08:08, Fabien COELHO wrote:
> About doc: I'd consider "generation" instead of "generating" as a 
> secondary index term.

We do use the "-ing" form for other secondary index terms.  It's useful
because the concatenation of primary and secondary term should usually
make a phrase of some sort.  The alternative would be "generation of",
but that doesn't seem clearly better.

>> (There is also precedent for redirecting the extension function to the
>> internal one by changing the SQL-level function definition using CREATE
>> OR REPLACE FUNCTION ... LANGUAGE INTERNAL.  But that seems more
>> complicated and would require a new extension version.  It could maybe
>> be included if the extension version is changed for other reasons.)
> 
> What about avoiding a redirection with something like:
> 
> Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid;

That seems very confusing.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Peter,

>> About doc: I'd consider "generation" instead of "generating" as a
>> secondary index term.
>
> We do use the "-ing" form for other secondary index terms.  It's useful
> because the concatenation of primary and secondary term should usually
> make a phrase of some sort.  The alternative would be "generation of",
> but that doesn't seem clearly better.

Ok, fine. I looked but did not find other instances of "generating".

>> What about avoiding a redirection with something like:
>>
>> Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid;
>
> That seems very confusing.

Dunno. Possibly. The user does not have to look at the implementation, and 
probably such code would deserve a comment.

The point is to avoid one call so as to perform the same (otherwise the 
pg_random_uuid would be slightly slower), and to ensure that it behaves 
the same, as it would be the very same function by construction.

I've switched the patch to ready anyway.

-- 
Fabien.

Hello Jose,

> Considering the later arguments on-list, I plan on submitting a more 
> elaborate patchset integrating the feedback received so far, and along the 
> following lines:
>
> - uuid type, v4 generation and uuid_version() in core
>
> - Provide a means to rename/supercede extensions keeping backwards 
> compatibility (i.e. uuid-ossp -> uuid, keep old code working)
>
> - Miscellaneous other functionality
>
> - Drop "dead" code
>
>   ...but I've tried to keep quiet so as not to disturb too much around 
> release time.
>
> I intend to continue working on this in late July, aiming for the following 
> commitfest (once more "urgent" patches will have landed)

Ok.

I've changed the patch status for this CF to "moved do next CF", and to 
"Waiting on author" there.

The idea is to go on in the same thread when you are ready.

-- 
Fabien.

On 2019-07-13 17:13, Fabien COELHO wrote:
>>> What about avoiding a redirection with something like:
>>>
>>> Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid;
>>
>> That seems very confusing.
> 
> Dunno. Possibly. The user does not have to look at the implementation, and 
> probably such code would deserve a comment.
> 
> The point is to avoid one call so as to perform the same (otherwise the 
> pg_random_uuid would be slightly slower), and to ensure that it behaves 
> the same, as it would be the very same function by construction.
> 
> I've switched the patch to ready anyway.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 7/14/19 9:40 PM, Peter Eisentraut wrote:
> On 2019-07-13 17:13, Fabien COELHO wrote:
>>>> What about avoiding a redirection with something like:
>>>>
>>>> Datum (* const pg_random_uuid)(PG_FUNCTION_ARGS) = gen_random_uuid;
>>>
>>> That seems very confusing.
>>
>> Dunno. Possibly. The user does not have to look at the implementation, and
>> probably such code would deserve a comment.
>>
>> The point is to avoid one call so as to perform the same (otherwise the
>> pg_random_uuid would be slightly slower), and to ensure that it behaves
>> the same, as it would be the very same function by construction.
>>
>> I've switched the patch to ready anyway.
> 
> committed

Small doc tweak suggestion - the pgcrypto docs [1] now say about gen_random_uuid():

     Returns a version 4 (random) UUID. (Obsolete, this function is now also
     included in core PostgreSQL.)

which gives the impression the code contains two versions of this function, the core
one and an obsolete one in pgcrypto. Per the commit message the situation is actually:

     The pgcrypto implementation now internally redirects to the built-in one.

Suggested wording improvement in the attached patch.

[1] https://www.postgresql.org/docs/devel/pgcrypto.html#id-1.11.7.34.9


Regards

Ian Barwick

-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

On 2019-Jul-13, Jose Luis Tallon wrote:

> Considering the later arguments on-list, I plan on submitting a more
> elaborate patchset integrating the feedback received so far, and along the
> following lines:
> 
> - uuid type, v4 generation and uuid_version() in core
> 
> - Provide a means to rename/supercede extensions keeping backwards
> compatibility (i.e. uuid-ossp -> uuid, keep old code working)

It is wholly unclear what this commitfest entry is all about; in the
thread there's a mixture about a new uuid_version(), some new v4 stuff
migrating from pgcrypto (which apparently was done), plus some kind of
mechanism to allow upgrading extension names; all this stemming from
feedback from the patch submitted in April.  But there hasn't been a new
patch in a long time, and there won't be a new patch version during the
current commitfest.  Therefore, I'm closing this entry as Returned with
Feedback.  The author(s) can create a new entry in a future commitfest
once they have a new patch.

I do suggest to keep such a patch restricted in scope.

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