Re: [PATCH] pg_sleep(interval)

Someone on IRC a while ago was complaining that there was no way to
specify an interval for pg_sleep, so I made one. Patch against today's
HEAD attached.

Usage: SELECT pg_sleep(interval '2 minutes');

I would add this to the next commitfest but I seem to be unable to log
in with my community account (I can log in to the wiki). Help appreciated.

> Usage: SELECT pg_sleep(interval '2 minutes');
>
> I would add this to the next commitfest but I seem to be unable to log
> in with my community account (I can log in to the wiki). Help appreciated.

Done.

--
Fabien.

On Thu, Aug 8, 2013 at 1:52 PM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> Someone on IRC a while ago was complaining that there was no way to
> specify an interval for pg_sleep, so I made one. Patch against today's
> HEAD attached.
>
> Usage: SELECT pg_sleep(interval '2 minutes');
>
> I would add this to the next commitfest but I seem to be unable to log
> in with my community account (I can log in to the wiki). Help appreciated.

Try changing your password on the main website. There was a bug a
while ago (a few months iirc) and if you changed your password or
created your account during that period, it would not work for the cf
app (but it would work for the wiki and some other sites).

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Thu, Aug 8, 2013 at 7:52 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> Someone on IRC a while ago was complaining that there was no way to
> specify an interval for pg_sleep, so I made one. Patch against today's
> HEAD attached.
>
> Usage: SELECT pg_sleep(interval '2 minutes');

The problem with this is that then pg_sleep would be overloaded, and
as we've found from previous experience (cf. pg_size_pretty) that can
make things that currently work start failing as ambiguous.

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

Except there are no data types that can be cast to both double and
interval currently.

On Fri, Aug 16, 2013 at 2:57 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> Except there are no data types that can be cast to both double and
> interval currently.

That, unfortunately, is not sufficient to avoid a problem.

rhaas=# create or replace function foo(double precision) returns
double precision as $$select $1$$ language sql;
CREATE FUNCTION
rhaas=# create or replace function foo(interval) returns interval as
$$select $1$$ language sql;
CREATE FUNCTION
rhaas=# select foo('123');
ERROR: function foo(unknown) is not unique
LINE 1: select foo('123');
^
HINT: Could not choose a best candidate function. You might need to
add explicit type casts.
rhaas=#

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

On 8/16/13 3:35 PM, Robert Haas wrote:
> On Fri, Aug 16, 2013 at 2:57 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
>> Except there are no data types that can be cast to both double and
>> interval currently.
>
> That, unfortunately, is not sufficient to avoid a problem.
>
> rhaas=# create or replace function foo(double precision) returns
> double precision as $$select $1$$ language sql;
> CREATE FUNCTION
> rhaas=# create or replace function foo(interval) returns interval as
> $$select $1$$ language sql;
> CREATE FUNCTION
> rhaas=# select foo('123');
> ERROR: function foo(unknown) is not unique
> LINE 1: select foo('123');
> ^
> HINT: Could not choose a best candidate function. You might need to
> add explicit type casts.

That example can be used as an argument against almost any kind of
overloading.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 8/16/13 3:35 PM, Robert Haas wrote:
>> On Fri, Aug 16, 2013 at 2:57 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
>>> Except there are no data types that can be cast to both double and
>>> interval currently.

>> That, unfortunately, is not sufficient to avoid a problem.

> That example can be used as an argument against almost any kind of
> overloading.

True. I think the gripe here is that pg_sleep('42') has worked for
many releases now, and if we add this patch then it would suddenly
stop working. How common is that usage likely to be (probably not
very), and how useful is it to have a version of pg_sleep that
takes an interval (probably also not very)?

Since the same effect can be had by writing a user-defined SQL function,
I'm a bit inclined to say that the value-added by having this as a
built-in function doesn't justify the risk of breaking existing apps.
It's a close call though, because both the risk and the value-added seem
rather small from here.

regards, tom lane

On 08/16/2013 04:52 PM, Tom Lane wrote:
> Since the same effect can be had by writing a user-defined SQL function,
> I'm a bit inclined to say that the value-added by having this as a
> built-in function doesn't justify the risk of breaking existing apps.
> It's a close call though, because both the risk and the value-added seem
> rather small from here.

Why not just call it pg_sleep_int()?

I, for one, would find it useful, but would be really unhappy about
about having to debug a bunch of old code to figure out what was broken.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Why not just call it pg_sleep_int()?

To me, that looks like something that would take an int. I suppose you
could call it pg_sleep_interval(), but that's getting pretty verbose.

The larger picture here though is that that's ugly as sin; it just flies
in the face of the fact that PG *does* have function overloading and we
do normally use it, not invent randomly-different function names to avoid
using it. We should either decide that this feature is worth the small
risk of breakage, or reject it. Not build a camel-designed-by-committee
because no one would speak up for consistency of design.

regards, tom lane

On 08/16/2013 05:15 PM, Tom Lane wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> Why not just call it pg_sleep_int()?
>
> To me, that looks like something that would take an int. I suppose you
> could call it pg_sleep_interval(), but that's getting pretty verbose.
>
> The larger picture here though is that that's ugly as sin; it just flies
> in the face of the fact that PG *does* have function overloading and we
> do normally use it, not invent randomly-different function names to avoid
> using it. We should either decide that this feature is worth the small
> risk of breakage, or reject it. Not build a camel-designed-by-committee
> because no one would speak up for consistency of design.

Well, if that's the alternative, I'd vote for taking it. For me,
personally, I think the usefulness of it would outstrip the number of
functions I'd have to debug.

For one thing, it's not like pg_sleep is exactly widely used, especially
not from languages like PHP which tend to treat every variable as a
string. So this is not going to be the kind of upgrade bomb that
pg_size_pretty was.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Fri, Aug 16, 2013 at 4:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> That example can be used as an argument against almost any kind of
> overloading.

Yep.

And that may be justified. We don't handle overloading particularly well.

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

> True. I think the gripe here is that pg_sleep('42') has worked for
> many releases now,

Maybe it could still work if pg_sleep(TEXT) is added as well?

--
Fabien.

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> Well, if that's the alternative, I'd vote for taking it. For me,
> personally, I think the usefulness of it would outstrip the number of
> functions I'd have to debug.

Agreed. Having it take an interval makes a whole lot more sense than
backing a decision that it should take a string-cast-to-integer.

Thanks,

Stephen

On 8/16/13 7:52 PM, Tom Lane wrote:
> I think the gripe here is that pg_sleep('42') has worked for
> many releases now, and if we add this patch then it would suddenly
> stop working. How common is that usage likely to be (probably not
> very), and how useful is it to have a version of pg_sleep that
> takes an interval (probably also not very)?

I think it's always going to be a problem going from a function with
only one signature to more than one. It's not going to be a problem
going from two to more.

For example, if you had foo(point) and much later you want to add
foo(box), someone might complain that foo('(1,2)') has worked for many
releases now, and how common is that use? If we had started out with
foo(point) and foo(line) simultaneously, this wouldn't have become a
problem.

This is quite a silly situation. I don't know a good answer, except
either ignoring the problem or requiring that any new function has at
least two overloaded variants. ;-)

> For example, if you had foo(point) and much later you want to add
> foo(box), someone might complain that foo('(1,2)') has worked for many
> releases now, and how common is that use? If we had started out with
> foo(point) and foo(line) simultaneously, this wouldn't have become a
> problem.

You may provide foo(TEXT) along foo(box) so that foo('(1,2)') works as it
did before.

--
Fabien.

Here is a review of the pg_sleep(INTERVAL) patch version 1:

- patch applies cleanly on current head

- the function documentation is updated and clear

- the function definition relies on a SQL function
to call the underlying pg_sleep(INT) function
I'm fine with this, especially as if someone calls this
function, he/she is not in a hurry:-)

- this one-liner implementation is straightforward

- the provided feature is simple, and seems useful.

- configure/make/make check work ok

However :

- the new function is *not* tested anywhere!

I would suggest simply to replace some pg_sleep(int) instances
by corresponding pg_sleep(interval) instances in the non
regression tests.

- some concerns have been raised that it breaks pg_sleep(TEXT)
which currently works thanks to the implicit TEXT -> INT cast.

I would suggest to add pg_sleep(TEXT) explicitely, like:

CREATE FUNCTION pg_sleep(TEXT) RETURNS VOID VOLATILE STRICT AS
$$ select pg_sleep($1::INTEGER) $$ LANGUAGE SQL;

That would be another one liner, to update the documentation and
to add some tests as well!

ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
upward-compatibility issue raised.

--
Fabien

On Fri, Sep 20, 2013 at 7:59 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> - the new function is *not* tested anywhere!
>
> I would suggest simply to replace some pg_sleep(int) instances
> by corresponding pg_sleep(interval) instances in the non
> regression tests.
>
> - some concerns have been raised that it breaks pg_sleep(TEXT)
> which currently works thanks to the implicit TEXT -> INT cast.
>
> I would suggest to add pg_sleep(TEXT) explicitely, like:
>
> CREATE FUNCTION pg_sleep(TEXT) RETURNS VOID VOLATILE STRICT AS
> $$ select pg_sleep($1::INTEGER) $$ LANGUAGE SQL;
>
> That would be another one liner, to update the documentation and
> to add some tests as well!
>
> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
> upward-compatibility issue raised.

I think that's ugly and I'm not one bit convinced it will resolve all
the upgrade-compatibility issues. Realistically, all sleeps are going
to be reasonably well measured in seconds anyway. If you want to
sleep for some other interval, convert that interval to a number of
seconds first.

Another problem is that, as written, this is vulnerable to search_path
hijacking attacks.

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

Hello Robert,

>> - some concerns have been raised that it breaks pg_sleep(TEXT)
>> which currently works thanks to the implicit TEXT -> INT cast.
>>
>> I would suggest to add pg_sleep(TEXT) explicitely, like:
>>
>> CREATE FUNCTION pg_sleep(TEXT) RETURNS VOID VOLATILE STRICT AS
>> $$ select pg_sleep($1::INTEGER) $$ LANGUAGE SQL;
>>
>> That would be another one liner, to update the documentation and
>> to add some tests as well!
>>
>> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
>> upward-compatibility issue raised.
>
> I think that's ugly and I'm not one bit convinced it will resolve all
> the upgrade-compatibility issues.

> Realistically, all sleeps are going to be reasonably well measured in
> seconds anyway.

I do not know that. From a "usual" dabatabase point of view, it does not
make much sense to slow down a database anyway, and this function is never
needed... so it really depends on the use case.

If someone want to simulate a long standing transaction to check its
effect on some features such as dumping data orreplication or whatever,
maybe pg_sleep(INTERVAL '5 hours') is nicer that pg_sleep(18000), if you
are not too good at dividing by 60, 3600 or 86400...

> If you want to sleep for some other interval, convert that interval to a
> number of seconds first.

You could say that for any use of INTERVAL. ISTM that the point if the
interval type is just to be more readable than a number of seconds to
express a delay.

> Another problem is that, as written, this is vulnerable to search_path
> hijacking attacks.

Yes, sure. I was not suggesting to create the function directly as above,
this is just the test I made to check whether it worked as I thought, i.e.
providing a TEXT version works and interacts properly with the INTEGER and
INTERVAL versions. My guess is that the function definition would be
inserted directly in pg_proc as other pg_* functions at initdb time.

--
Fabien.

On 09/20/2013 01:59 PM, Fabien COELHO wrote:
>
> Here is a review of the pg_sleep(INTERVAL) patch version 1:

Thank you for looking at it.

>
> - the new function is *not* tested anywhere!
>
> I would suggest simply to replace some pg_sleep(int) instances
> by corresponding pg_sleep(interval) instances in the non
> regression tests.

Attached is a rebased patch that adds a test as you suggest.

> - some concerns have been raised that it breaks pg_sleep(TEXT)
> which currently works thanks to the implicit TEXT -> INT cast.

There is no pg_sleep(text) function and the cast is unknown->double
precision.

>
> I would suggest to add pg_sleep(TEXT) explicitely, like:
>
> CREATE FUNCTION pg_sleep(TEXT) RETURNS VOID VOLATILE STRICT AS
> $$ select pg_sleep($1::INTEGER) $$ LANGUAGE SQL;
>
> That would be another one liner, to update the documentation and
> to add some tests as well!
>
> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
> upward-compatibility issue raised.
>

I don't like this idea at all. If we don't want to break compatibility
for callers that quote the value, I would rather the patch be rejected
outright.

--
Vik

Hello,

> There is no pg_sleep(text) function and the cast is unknown->double
> precision.

My mistake.

As I understand it, pg_sleep('12') currently works and would not anymore
once your patch is applied. That is the concern raised by Robert Haas.

>> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
>> upward-compatibility issue raised.
>
> I don't like this idea at all. If we don't want to break compatibility
> for callers that quote the value, I would rather the patch be rejected
> outright.

That was just a suggestion, and I was trying to help. ISTM that if
Robert's concern is not addressed one way or another, you will just get
"rejected" on this basis.

--
Fabien.

On 09/22/2013 02:17 PM, Fabien COELHO wrote:
>
> Hello,
>
>> There is no pg_sleep(text) function and the cast is unknown->double
>> precision.
>
> My mistake.
>
> As I understand it, pg_sleep('12') currently works and would not
> anymore once your patch is applied. That is the concern raised by
> Robert Haas.

That is correct.

>
>>> ISTM that providing "pg_sleep(TEXT)" cleanly resolves the
>>> upward-compatibility issue raised.
>>
>> I don't like this idea at all. If we don't want to break compatibility
>> for callers that quote the value, I would rather the patch be rejected
>> outright.
>
> That was just a suggestion, and I was trying to help. ISTM that if
> Robert's concern is not addressed one way or another, you will just
> get "rejected" on this basis.
>

Yes, I understand you are trying to help, and I appreciate it! My
opinion, and that of others as well from the original thread, is that
this patch should either go in as is and break that one case, or not go
in at all. I'm fine with either (although clearly I would prefer it
went in otherwise I wouldn't have written the patch).

--
Vik

On 09/30/2013 01:47 PM, Vik Fearing wrote:
> Yes, I understand you are trying to help, and I appreciate it! My
> opinion, and that of others as well from the original thread, is that
> this patch should either go in as is and break that one case, or not go
> in at all. I'm fine with either (although clearly I would prefer it
> went in otherwise I wouldn't have written the patch).

I see this is marked as rejected in the commitfest app, but I don't see
any note about who did it or why. I don't believe there is consensus
for rejection on this list. In fact I think the opposite is true.

May we have an explanation please from the person who rejected this
without comment?

--
Vik

Hello Vik,

>> Yes, I understand you are trying to help, and I appreciate it! My
>> opinion, and that of others as well from the original thread, is that
>> this patch should either go in as is and break that one case, or not go
>> in at all. I'm fine with either (although clearly I would prefer it
>> went in otherwise I wouldn't have written the patch).
>
> I see this is marked as rejected in the commitfest app, but I don't see
> any note about who did it or why. I don't believe there is consensus
> for rejection on this list. In fact I think the opposite is true.
>
> May we have an explanation please from the person who rejected this
> without comment?

I did it, on the basis that you stated that you prefered not adding
pg_sleep(TEXT) to answer Robert Haas concern about preserving
pg_sleep('10') current functionality, and that no other solution was
suggested to tackle this issue.

If I'm mistaken, feel free to change the state back to what is
appropriate.

My actual opinion is that breaking pg_sleep('10') is no big deal, but I'm
nobody here, and Robert is somebody, so I think that his concern must be
addressed.

--
Fabien.

On 10/16/2013 10:57 AM, Fabien COELHO wrote:
>
> Hello Vik,
>
>> I see this is marked as rejected in the commitfest app, but I don't see
>> any note about who did it or why. I don't believe there is consensus
>> for rejection on this list. In fact I think the opposite is true.
>>
>> May we have an explanation please from the person who rejected this
>> without comment?
>
> I did it, on the basis that you stated that you prefered not adding
> pg_sleep(TEXT) to answer Robert Haas concern about preserving
> pg_sleep('10') current functionality, and that no other solution was
> suggested to tackle this issue.

The suggested solution is to ignore the issue.

> If I'm mistaken, feel free to change the state back to what is
> appropriate.

I'm not really sure what the proper workflow is for marking a patch as
rejected, actually. I wouldn't mind some clarification on this from the
CFM or somebody.

In the meantime I've set it to Ready for Committer because in my mind
there is a consensus for it (see below) and you don't appear to have
anything more to say about the patch except for the do-we/don't-we issue.

> My actual opinion is that breaking pg_sleep('10') is no big deal, but
> I'm nobody here, and Robert is somebody, so I think that his concern
> must be addressed.

Tom Lane is somebody, too, and his opinion is to break it or reject it
although he refrains from picking a side[1]. Josh Berkus and Stephen
Frost are both somebodies and they are on the "break it" side[2][3].
Peter Eisentraut gave no opinion at all but did say that Robert's
argument was not very good. I am for it because I wrote the patch, and
you seem not to care. So the way I see it we have:

For: Josh, Stephen, me
Against: Robert
Neutral: Tom, you

I don't know if that's enough of a consensus to commit it, but I do
think it's not nearly enough of a consensus to reject it.

[1] http://www.postgresql.org/message-id/16727.1376697147%40sss.pgh.pa.us
[2] http://www.postgresql.org/message-id/520EC584.3050805@agliodbs.com
[3]
http://www.postgresql.org/message-id/20130820013033.GU2706@tamriel.snowman.net

--
Vik

* Vik Fearing (vik(dot)fearing(at)dalibo(dot)com) wrote:
> I don't know if that's enough of a consensus to commit it, but I do
> think it's not nearly enough of a consensus to reject it.

This is actually a problem that I think we have today- the expectation
that *everyone* has to shoot down an idea for it to be rejected, but
one individual saying "oh, that's a good idea" means it must be
committed.

That's not how it works and there's no notion of "pending further
discussion" in the CF; imv that equates to "returned with feedback."
Marking this patch as 'Ready for Committer' when multiple committers
have already commented on it doesn't strike me as moving things forward
either.

As it relates to this specific patch for this CF, I'd go with 'Returned
with Feedback' and encourage you to consider the arguments for and
against, and perhaps try to find existing usage which would break due to
this change and consider the impact of changing it. For example, what
do the various languages and DB abstraction layers do today? Would
users of Hibernate likely be impacted or no? What about PDO?
Personally, I'm still on-board with the change in general, but it'd
really help to know that normal/obvious usage through various languages
won't be busted by the change.

Thanks,

Stephen

On 2013-10-16 11:18:55 -0400, Stephen Frost wrote:
> This is actually a problem that I think we have today- the expectation
> that *everyone* has to shoot down an idea for it to be rejected, but
> one individual saying "oh, that's a good idea" means it must be
> committed.

But neither does a single objection mean it cannot get committed. I
don't see either scenario being present here though.

Greetings,

Andres Freund

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

On Wed, Oct 16, 2013 at 11:18 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Vik Fearing (vik(dot)fearing(at)dalibo(dot)com) wrote:
>> I don't know if that's enough of a consensus to commit it, but I do
>> think it's not nearly enough of a consensus to reject it.
>
> This is actually a problem that I think we have today- the expectation
> that *everyone* has to shoot down an idea for it to be rejected, but
> one individual saying "oh, that's a good idea" means it must be
> committed.
>
> That's not how it works and there's no notion of "pending further
> discussion" in the CF; imv that equates to "returned with feedback."
> Marking this patch as 'Ready for Committer' when multiple committers
> have already commented on it doesn't strike me as moving things forward
> either.
>
> As it relates to this specific patch for this CF, I'd go with 'Returned
> with Feedback' and encourage you to consider the arguments for and
> against, and perhaps try to find existing usage which would break due to
> this change and consider the impact of changing it. For example, what
> do the various languages and DB abstraction layers do today? Would
> users of Hibernate likely be impacted or no? What about PDO?
> Personally, I'm still on-board with the change in general, but it'd
> really help to know that normal/obvious usage through various languages
> won't be busted by the change.

I generally agree, although I'm not as sanguine as you about the
overall prospects for the patch. The bottom line is that there are
cases, like pg_sleep('42') that will be broken by this, and if you fix
those by some hack there will be other cases that break instead.
Recall what happened with pg_size_pretty(), which did not turn out
nearly as well as I thought it would, though I advocated for it at the
time. There's just no such thing as a free lunch: we *can't* change
the API for functions that have been around for many releases without
causing some pain for users that are depending on those functions.

Now, of course, sometimes it's worth it. We can and should change
things when there's enough benefit to justify the pain that comes from
breaking backward compatibility. But I don't see that as being the
case here. Anyone who actually wants this in their environment can
add the overloaded function in their environment with a one-line SQL
function: pg_sleep(interval) as proposed here is just
pg_sleep(extract(epoch from now() + $1) - extract(epoch from now())).
Considering how easy that is, I don't see why we need to have it in
core.

In general, I'm a big fan of composibility as a design principle. If
you have a function that does A and a function that does B, it's
reasonable to say that people should use them together, rather than
providing a third function that does AB. Otherwise, you just end up
with too many functions. Of course, there are exceptions: if A and B
are almost always done together, a convenience function may indeed be
warranted. But I don't see how you can argue that here; there are
doubtless many existing users of pg_sleep(double) that are perfectly
happy with the existing behavior.

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

On 10/16/2013 08:18 AM, Stephen Frost wrote:
> As it relates to this specific patch for this CF, I'd go with 'Returned
> with Feedback' and encourage you to consider the arguments for and
> against, and perhaps try to find existing usage which would break due to
> this change and consider the impact of changing it. For example, what
> do the various languages and DB abstraction layers do today? Would
> users of Hibernate likely be impacted or no? What about PDO?
> Personally, I'm still on-board with the change in general, but it'd
> really help to know that normal/obvious usage through various languages
> won't be busted by the change.

I'm fairly sure that the only language likely to be impacted chronically
is PHP. Java, Ruby and Python, as a rule, properly data-type their
parameters; PHP is the only language I know of where developers *and
frameworks* chronically pass everything as a string. IIRC, the primary
complainers when we removed a bunch of implicit casts in 8.3 were PHP devs.

One thing to keep in mind, though, is that few developers actually use
pg_sleep(), and I'd be surprised to find in in any canned web framework.
Generally, if a web developer is going to sleep, they do it in the
application. So we're really only talking about stored procedure
writers here. And, as a rule, we can expect stored procedure writers to
not gratuitously use strings in place of integers.

We generally don't bounce PLpgSQL features which break unsupported
syntax in PLpgSQL, since we expect people who write functions to have a
better knowledge of SQL syntax than people who don't. I think
pg_sleep(interval) falls under the same test. So I'd vote to accept it.

Also, as Tom pointed out, at some point we have to either say we really
support overloading or we don't.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Oct 16, 2013 at 1:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Also, as Tom pointed out, at some point we have to either say we really
> support overloading or we don't.

We clearly do support overloading. I don't think that's at issue.
But as we all know, using it can cause formerly unambiguous queries to
become ambiguous and stop working.

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

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> Anyone who actually wants this in their environment can
> add the overloaded function in their environment with a one-line SQL
> function: pg_sleep(interval) as proposed here is just
> pg_sleep(extract(epoch from now() + $1) - extract(epoch from now())).

You're making it sound way harder than it is.  Why not just:

create function my_sleep(delay interval)
  returns void language sql
  as 'select pg_sleep(extract(epoch from $1))';

... or,  of course, named to match the existing function.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/16/13 2:40 PM, Robert Haas wrote:
> On Wed, Oct 16, 2013 at 1:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Also, as Tom pointed out, at some point we have to either say we really
>> support overloading or we don't.
>
> We clearly do support overloading. I don't think that's at issue.
> But as we all know, using it can cause formerly unambiguous queries to
> become ambiguous and stop working.

But that's not really what this is. It's one thing to be wary about
adding foo(bigint, int, smallint) when there are already three other
permutations in the system. (At least in other languages, compilers
will give you warnings or errors when this creates an ambiguity, so
there's no guessing.) But the problem here is that if there already is a

foo(type1)

then the proposal to add

foo(type2)

can always be shot down by

"But this will break foo('type1val')."

That can't be in the spirit of overloading.

The only way to fix this is that at the time when you add foo(type1) you
need to prevent people from being able to call foo('type1val') and
instead require the full syntax foo(type1 'type1val'). The only way to
do that, I think, is to add some other foo(type3) at the same time.
There is just something wrong with that.

On Wed, Oct 16, 2013 at 4:34 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> Anyone who actually wants this in their environment can
>> add the overloaded function in their environment with a one-line SQL
>> function: pg_sleep(interval) as proposed here is just
>> pg_sleep(extract(epoch from now() + $1) - extract(epoch from now())).
>
> You're making it sound way harder than it is. Why not just:
>
> create function my_sleep(delay interval)
> returns void language sql
> as 'select pg_sleep(extract(epoch from $1))';
>
> ... or, of course, named to match the existing function.

Because that might or might not do the right thing if the interval is 1 month.

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

Hello Vik,

> For: Josh, Stephen, me
> Against: Robert
> Neutral: Tom, you

For the record, I'm not neutral, I'm *FOR*. I reviewed it and said that I
think it is fine. But I'm still nobody here:-)

My experience at trying to pass minor patches shows that the basic
behavior is conservatism. Maybe this is necessary to the stability of the
project, but this is really annoying for pretty small changes on side
tools, and I think that it tends to over-conservatism and ampers good
will. You have to argue a lot about trivial things. My ratio for passing
very minor patches on pgbench for instance is 1:3 or worst, 1 unit
programming and testing versus 3 units writing mails to argue about this
and that. Maybe the ratio is better for big important patches. Your patch
is quite representative, 1 line of trivial code, a few lines of tests and
docs, and how many lines and time at writing mails?

> I don't know if that's enough of a consensus to commit it, but I do
> think it's not nearly enough of a consensus to reject it.

My guess is that it won't be committed if there is a single "but it might
break one code or surprise one user somewhere in the universe", but I wish
I'll be proven wrong. IMO, "returned with feedback" on a 1 liner is really
akin to "rejected".

--
Fabien.

On Wed, Oct 16, 2013 at 5:16 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 10/16/13 2:40 PM, Robert Haas wrote:
>> On Wed, Oct 16, 2013 at 1:26 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>>> Also, as Tom pointed out, at some point we have to either say we really
>>> support overloading or we don't.
>>
>> We clearly do support overloading. I don't think that's at issue.
>> But as we all know, using it can cause formerly unambiguous queries to
>> become ambiguous and stop working.
>
> But that's not really what this is. It's one thing to be wary about
> adding foo(bigint, int, smallint) when there are already three other
> permutations in the system. (At least in other languages, compilers
> will give you warnings or errors when this creates an ambiguity, so
> there's no guessing.) But the problem here is that if there already is a
>
> foo(type1)
>
> then the proposal to add
>
> foo(type2)
>
> can always be shot down by
>
> "But this will break foo('type1val')."
>
> That can't be in the spirit of overloading.
>
> The only way to fix this is that at the time when you add foo(type1) you
> need to prevent people from being able to call foo('type1val') and
> instead require the full syntax foo(type1 'type1val'). The only way to
> do that, I think, is to add some other foo(type3) at the same time.
> There is just something wrong with that.

Actually, this could be fixed by having a way to declare one of the
overloaded functions as the preferred option and resolving ambiguous
calls in favor of the highest-priority function. In fact,
EnterpriseDB has added just such an option to Advanced Server 9.3, and
it fixes several longstanding difficult choices between being
Oracle-compatible and being PostgreSQL-compatible; we're now more
compatible with both. If we had that option in PostgreSQL, we could
declare the existing function as the preferred option, add the new
one, and be pretty much assured of not breaking anything.

However, I've learned from experience that submitting patches to
improve PostgreSQL's only-marginally-usable ambiguous function
resolution code is an exercise in futility. My last attempt ended
with a clear majority of people telling me that they liked failing to
call *the only function called foo* when confronted with a call that
was clearly intended to reference that function. As nearly as I can
tell, people like the idea of such unnecessary failures only in
theory. As soon as it comes to any practical case (like this one),
people start looking for workarounds. Right now there aren't any good
ones, and the reason for that is simple: we refuse to entertain adding
them.

Just sayin'.

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

On 10/17/2013 10:03 AM, Fabien COELHO wrote:
> My guess is that it won't be committed if there is a single "but it
> might break one code or surprise one user somewhere in the universe",
> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
> liner is really akin to "rejected".

I have attached here an entirely new patch (new documentation and
everything) that should please everyone. It no longer overloads
pg_sleep(double precision) but instead add two new functions:

* pg_sleep_for(interval)
* pg_sleep_until(timestamp with time zone)

Because it's no longer overloading the original pg_sleep, Robert's
ambiguity objection is no more.

Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');

If people like this, I'll reject the current patch and add this one to
the next commitfest.

Opinions?

--
Vik

On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
>> My guess is that it won't be committed if there is a single "but it
>> might break one code or surprise one user somewhere in the universe",
>> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
>> liner is really akin to "rejected".
>
> I have attached here an entirely new patch (new documentation and
> everything) that should please everyone. It no longer overloads
> pg_sleep(double precision) but instead add two new functions:
>
> * pg_sleep_for(interval)
> * pg_sleep_until(timestamp with time zone)
>
> Because it's no longer overloading the original pg_sleep, Robert's
> ambiguity objection is no more.
>
> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>
> If people like this, I'll reject the current patch and add this one to
> the next commitfest.

I find that naming relatively elegant. However, you've got to
schema-qualify every function and operator used in the definitions, or
you're creating a search-path security vulnerability.

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

On 10/17/2013 02:42 PM, Robert Haas wrote:
> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
>> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
>>> My guess is that it won't be committed if there is a single "but it
>>> might break one code or surprise one user somewhere in the universe",
>>> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
>>> liner is really akin to "rejected".
>> I have attached here an entirely new patch (new documentation and
>> everything) that should please everyone. It no longer overloads
>> pg_sleep(double precision) but instead add two new functions:
>>
>> * pg_sleep_for(interval)
>> * pg_sleep_until(timestamp with time zone)
>>
>> Because it's no longer overloading the original pg_sleep, Robert's
>> ambiguity objection is no more.
>>
>> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>>
>> If people like this, I'll reject the current patch and add this one to
>> the next commitfest.
> I find that naming relatively elegant. However, you've got to
> schema-qualify every function and operator used in the definitions, or
> you're creating a search-path security vulnerability.
>

Good catch. Updated patch attached.

--
Vik

Hello Vik,

> I have attached here an entirely new patch (new documentation and
> everything) that should please everyone. It no longer overloads
> pg_sleep(double precision) but instead add two new functions:
>
> * pg_sleep_for(interval)
> * pg_sleep_until(timestamp with time zone)
>
> Because it's no longer overloading the original pg_sleep, Robert's
> ambiguity objection is no more.
>
> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>
> If people like this, I'll reject the current patch and add this one to
> the next commitfest.
>
> Opinions?

I liked overloading...

Anyway, I have not looked at the patch in details, but both functions
seems useful at least if you need to sleep, and the solution is
reasonable.

I also like "pg_sleep_until('tomorrow');" that I guess would work, but I'm
not sure of any practical use case for this one. Do you have an example
where it makes sense?

--
Fabien.

Robert Haas escribió:

> Actually, this could be fixed by having a way to declare one of the
> overloaded functions as the preferred option and resolving ambiguous
> calls in favor of the highest-priority function. In fact,
> EnterpriseDB has added just such an option to Advanced Server 9.3, and
> it fixes several longstanding difficult choices between being
> Oracle-compatible and being PostgreSQL-compatible; we're now more
> compatible with both.

How does this work?

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

On Thu, Oct 17, 2013 at 9:51 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas escribió:
>> Actually, this could be fixed by having a way to declare one of the
>> overloaded functions as the preferred option and resolving ambiguous
>> calls in favor of the highest-priority function. In fact,
>> EnterpriseDB has added just such an option to Advanced Server 9.3, and
>> it fixes several longstanding difficult choices between being
>> Oracle-compatible and being PostgreSQL-compatible; we're now more
>> compatible with both.
>
> How does this work?

In Advanced Server, we've added implicit casts between some data types
between which PostgreSQL does not have implicit casts. We do this
because Oracle implicitly casts between those data types, and having
similar casts allows function calls that would succeed on Oracle to
also succeed on Advanced Server. Unfortunately, it also renders some
operators, particularly textanycat and anytextcat, ambiguous. In
existing releases of Advanced Server, we handled that by removing
those operators. This creates some breakage in edge cases:
concatenation with text still works fine for the data types for which
we added implicit casts to text, but for other data types it fails
where it would have succeeded in PostgreSQL.

In Advanced Server 9.3, we added a new pg_proc column called
proisweak. When a function or operator call would otherwise be
rejected as ambiguous, we check whether all but one of the remaining
candidates are marked proisweak; if so, we select the non-weak
candidate. Advanced Server 9.3 now marks the textanycat and
anytextcat operators as weak rather than removing them; this allows
type-with-an-implicit-cast-to-text || text to work, because we now
have a way to prefer implicit cast + textcat to anytextcat.

Obviously, the implicit casts are not for PostgreSQL and would be
rightly rejected here, but I am not sure that the ability to prefer
one function or operator over others in an overloading situation is
such a bad idea. So far, our internal testing seems to show that it
works well and doesn't break things.

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

On 10/17/13 8:06 AM, Robert Haas wrote:
> Actually, this could be fixed by having a way to declare one of the
> overloaded functions as the preferred option and resolving ambiguous
> calls in favor of the highest-priority function. In fact,
> EnterpriseDB has added just such an option to Advanced Server 9.3, and
> it fixes several longstanding difficult choices between being
> Oracle-compatible and being PostgreSQL-compatible; we're now more
> compatible with both. If we had that option in PostgreSQL, we could
> declare the existing function as the preferred option, add the new
> one, and be pretty much assured of not breaking anything.

That might be worth discussing. I'd prefer somehow getting rid of the
unknown literals/type, but that's a different discussion.

> However, I've learned from experience that submitting patches to
> improve PostgreSQL's only-marginally-usable ambiguous function
> resolution code is an exercise in futility. My last attempt ended
> with a clear majority of people telling me that they liked failing to
> call *the only function called foo* when confronted with a call that
> was clearly intended to reference that function. As nearly as I can
> tell, people like the idea of such unnecessary failures only in
> theory. As soon as it comes to any practical case (like this one),
> people start looking for workarounds. Right now there aren't any good
> ones, and the reason for that is simple: we refuse to entertain adding
> them.

Well, that was a proposal to make things more loose, and it's reasonable
to have issues with that. On the other hand, making things more strict
is obviously prone to break existing code. So it's indeed difficult to
make any changes either way.

Robert,

> Obviously, the implicit casts are not for PostgreSQL and would be
> rightly rejected here, but I am not sure that the ability to prefer
> one function or operator over others in an overloading situation is
> such a bad idea. So far, our internal testing seems to show that it
> works well and doesn't break things.

Hmmm. Is this better to do on the cast level or the function level?
For the case discussed, it would be sufficient to have a way to mark a
particular function signature as "preferred" in cases of ambiguity, and
that would be a lot less likely to have side effects. Mind you, fixing
the cast in general would fix far more annoying cases, but I also see it
as something which would be very hard to get correct ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Fabien,

> My experience at trying to pass minor patches shows that the basic
> behavior is conservatism. Maybe this is necessary to the stability of
> the project, but this is really annoying for pretty small changes on
> side tools, and I think that it tends to over-conservatism and ampers
> good will. You have to argue a lot about trivial things. My ratio for
> passing very minor patches on pgbench for instance is 1:3 or worst, 1
> unit programming and testing versus 3 units writing mails to argue about
> this and that. Maybe the ratio is better for big important patches. Your
> patch is quite representative, 1 line of trivial code, a few lines of
> tests and docs, and how many lines and time at writing mails?

This is, personally, the *entire* reason I've been arguing for this
patch. I see the arguments against it as being a case of unnecessary
conservatism, and cynically realize that if a well-known major
contributor had proposed it, the patch would be committed already.

Our project has a serious, chronic problem with giving new
patch-submitters a bad experience, and this patch is a good example of
that. The ultimate result is that people go off to contribute to other
projects where submissions are easier and the rules for what gets
accepted are relatively transparent.

Personally, I don't care about pg_sleep(interval) really. But I do care
that a minor contributor be able to submit and have accepted a patch of
clear general usability, and not get it rejected on the basis of "it
might break something for someone even though we don't have a clear idea
who".

Now, I do think the argument of "we don't really need pg_sleep(interval)
because it's trivial to do yourself" has some merit, and that would be a
good reason to argue acceptance or not. However, to date that has not
been the topic of discussion.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Oct 17, 2013 at 12:45 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> Obviously, the implicit casts are not for PostgreSQL and would be
>> rightly rejected here, but I am not sure that the ability to prefer
>> one function or operator over others in an overloading situation is
>> such a bad idea. So far, our internal testing seems to show that it
>> works well and doesn't break things.
>
> Hmmm. Is this better to do on the cast level or the function level?
> For the case discussed, it would be sufficient to have a way to mark a
> particular function signature as "preferred" in cases of ambiguity, and
> that would be a lot less likely to have side effects. Mind you, fixing
> the cast in general would fix far more annoying cases, but I also see it
> as something which would be very hard to get correct ...

This is of course to some degree a matter of opinion. The ankle bone
is connected to the leg bone, and the leg bone is connected to the
knee bone, and all that. It's very legitimate to think that we can
change the system in a variety of places to fix any given problem.

But if you're asking my opinion, I think doing it on the function
level is a whole lot better and easier to get right. A flag like the
one I mentioned here can be set for one particular function with the
absolute certainty that behavior will not change for any function with
some other name. That type of surety is pretty much impossible to get
with casts.

It's also not clear to me that the rules are logically related to the
input data type. We might prefer to choose pg_sleep(double) over
pg_sleep(interval) on backwards-compatibility grounds, but some other
function might be in the opposite situation. You can't really get a
lot of fine-grained control with casting here.

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

On 10/17/2013 10:01 AM, Robert Haas wrote:
> But if you're asking my opinion, I think doing it on the function
> level is a whole lot better and easier to get right. A flag like the
> one I mentioned here can be set for one particular function with the
> absolute certainty that behavior will not change for any function with
> some other name. That type of surety is pretty much impossible to get
> with casts.

The other argument for doing it at the function level is that we could
then expose it to users, who could use it to manage their own overloaded
functions. We would NOT want to encourage users to mess with cast
precedence, because it would be impossible for them to achieve their
desired result that way.

On the other hand, prioritization at the function level likely wouldn't
help us with operators at all, because there the cast has to be chosen
before we choose a function. So if we pursued the function route, then
we'd eventually want to add a "preferred" flag for operators too. Which
would be a lot more trouble, because it would affect the planner, but at
least that would be a seperate step.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Oct 17, 2013 at 12:59 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> Now, I do think the argument of "we don't really need pg_sleep(interval)
> because it's trivial to do yourself" has some merit, and that would be a
> good reason to argue acceptance or not. However, to date that has not
> been the topic of discussion.

I've made that exact argument several times on this thread. For example:

http://www.postgresql.org/message-id/CA+TgmobKneq=f9e8TzYwG6haoTZxOZPvJqh14mpb9f+XLv67ZQ@mail.gmail.com

I've been focusing on the backward compatibility issue mostly BECAUSE
I don't think the feature has much incremental value. If logical
replication or parallel query required breaking pg_sleep('42'), I
wouldn't be objecting. I'm sorry if that wasn't clear, and I further
apologize if you think I'm being too hard on a new patch submitter.

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

On Thu, Oct 17, 2013 at 1:10 PM, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
> On 10/17/2013 10:01 AM, Robert Haas wrote:
>> But if you're asking my opinion, I think doing it on the function
>> level is a whole lot better and easier to get right. A flag like the
>> one I mentioned here can be set for one particular function with the
>> absolute certainty that behavior will not change for any function with
>> some other name. That type of surety is pretty much impossible to get
>> with casts.
>
> The other argument for doing it at the function level is that we could
> then expose it to users, who could use it to manage their own overloaded
> functions. We would NOT want to encourage users to mess with cast
> precedence, because it would be impossible for them to achieve their
> desired result that way.
>
> On the other hand, prioritization at the function level likely wouldn't
> help us with operators at all, because there the cast has to be chosen
> before we choose a function. So if we pursued the function route, then
> we'd eventually want to add a "preferred" flag for operators too. Which
> would be a lot more trouble, because it would affect the planner, but at
> least that would be a seperate step.

Actually the operator resolution code is very much parallel to the
function resolution code. I am quite sure in Advanced Server we only
needed to add proisweak to handle both cases; unless I'm quite
mistaken, we did not add oprisweak.

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

Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> Our project has a serious, chronic problem with giving new
> patch-submitters a bad experience, and this patch is a good
> example of that.

Perhaps; but it has also been an example of the benefits of having
tight review.  IMO, pg_sleep_for() and pg_sleep_until() are better
than the initial proposal.  For one thing, since each accepts a
specific type, it allows for cleaner syntax.  These are not only
unambiguous, they are easier to code and read than what was
originally proposed:

select pg_sleep_for('10 minutes');
select pg_sleep_until('tomorrow 05:00');

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/17/2013 08:36 PM, Kevin Grittner wrote:
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
>> Our project has a serious, chronic problem with giving new
>> patch-submitters a bad experience, and this patch is a good
>> example of that.
> Perhaps; but it has also been an example of the benefits of having
> tight review.

FWIW, I agree. I have been impressed by the rigorous review process of
this project ever since I started following it.

> IMO, pg_sleep_for() and pg_sleep_until() are better
> than the initial proposal.

I agree here, as well. I was quite pleased with myself when I thought
of it, and I would not have thought of it had it not been for all the
pushback I received. Whether it goes in or not (I hope it does), I am
happy with the process.

> For one thing, since each accepts a
> specific type, it allows for cleaner syntax. These are not only
> unambiguous, they are easier to code and read than what was
> originally proposed:
>
> select pg_sleep_for('10 minutes');
> select pg_sleep_until('tomorrow 05:00');

These are pretty much exactly the examples I put in my documentation.

--
Vik

On 10/17/2013 06:59 PM, Josh Berkus wrote:
> Our project has a serious, chronic problem with giving new
> patch-submitters a bad experience, and this patch is a good example of
> that. The ultimate result is that people go off to contribute to other
> projects where submissions are easier and the rules for what gets
> accepted are relatively transparent.

That may be true, but it depends on the contributor. I would much
rather be told that my contribution is not up to snuff than what
happened on another project I recently tried to contribute to for the
first time.

A parser refactoring broke my code. I reported it and it was promptly
fixed. When the fix came up for review, I said it needed a regression
test to prevent it from happening again and I was told by the author
that such a test would be "flimsy" and it went on to be committed (by
that same guy) without one. I'm undecided whether I'll be contributing
there any further.

The rigor here makes me want to try and try again.

--
Vik

On 10/17/2013 01:41 PM, Vik Fearing wrote:
>> > Perhaps; but it has also been an example of the benefits of having
>> > tight review.
> FWIW, I agree. I have been impressed by the rigorous review process of
> this project ever since I started following it.
>

OK, good! That makes me feel better.

So, I surveyed 30 members of the San Francisco PostgreSQL User Group
last night. Out of the 30:

4 had ever used pg_sleep(), and those four included Jeff Davis and Peter
G. I asked the remaining two about the new versions of pg_sleep, and
they were more interested in pg_sleep_until(), and not particularly
interested in pg_sleep(interval).

So, to my mind backwards compatibility (the ambiguity issue) is
insignificant because there are so few users of pg_sleep(), but there
are serious questions about the demand for improvements on pg_sleep for
that reason.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 10/17/13 12:10 PM, Josh Berkus wrote:
> On 10/17/2013 10:01 AM, Robert Haas wrote:
>> But if you're asking my opinion, I think doing it on the function
>> level is a whole lot better and easier to get right. A flag like the
>> one I mentioned here can be set for one particular function with the
>> absolute certainty that behavior will not change for any function with
>> some other name. That type of surety is pretty much impossible to get
>> with casts.
>
> The other argument for doing it at the function level is that we could
> then expose it to users, who could use it to manage their own overloaded
> functions. We would NOT want to encourage users to mess with cast
> precedence, because it would be impossible for them to achieve their
> desired result that way.
>
> On the other hand, prioritization at the function level likely wouldn't
> help us with operators at all, because there the cast has to be chosen
> before we choose a function. So if we pursued the function route, then
> we'd eventually want to add a "preferred" flag for operators too. Which
> would be a lot more trouble, because it would affect the planner, but at
> least that would be a seperate step.

Yeah, but hasn't every case of this that we've run into been directly related to casting problems, and not function or operator preference?

ISTM that exposing the idea of function priority to users is opening a massive Pandora's box...

Something else I'm wondering is if priority should actually be something that's numbered instead of just a boolean. I can see far more logic to implicitly casting text to double than I can text to interval, but if a cast to double won't actually get you where you want and a cast to interval will... Maybe it's possible to account for all those cases with just a boolean... maybe not.
--
Jim C. Nasby, Data Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On 10/17/13 4:01 PM, Vik Fearing wrote:
> On 10/17/2013 06:59 PM, Josh Berkus wrote:
>> Our project has a serious, chronic problem with giving new
>> patch-submitters a bad experience, and this patch is a good example of
>> that. The ultimate result is that people go off to contribute to other
>> projects where submissions are easier and the rules for what gets
>> accepted are relatively transparent.
>
> That may be true, but it depends on the contributor. I would much
> rather be told that my contribution is not up to snuff than what
> happened on another project I recently tried to contribute to for the
> first time.
>
> A parser refactoring broke my code. I reported it and it was promptly
> fixed. When the fix came up for review, I said it needed a regression
> test to prevent it from happening again and I was told by the author
> that such a test would be "flimsy" and it went on to be committed (by
> that same guy) without one. I'm undecided whether I'll be contributing
> there any further.
>
> The rigor here makes me want to try and try again.

ISTM the big issue with new contributors is our methodology is rather different from most other projects, and if you don't understand that you're likely to end up with negativity towards contributing here. Specifically:

- We place a heavy, HEAVY emphasis on discussion, to the point that you can easily spend 50x more time on discussing a feature over implementing it.
- We place a very heavy emphasis on "quality", be that testing, not breaking backwards compatability, etc, etc.

I agree with Vik; I think the way we develop is a feature and not a bug. But I also think we need to do everything we can to enlighten new contributors so they don't walk away with a bad taste in their mouth.
--
Jim C. Nasby, Data Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Fri, Oct 18, 2013 at 7:21 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> Yeah, but hasn't every case of this that we've run into been directly
> related to casting problems, and not function or operator preference?

No.

> Something else I'm wondering is if priority should actually be something
> that's numbered instead of just a boolean. I can see far more logic to
> implicitly casting text to double than I can text to interval, but if a cast
> to double won't actually get you where you want and a cast to interval
> will... Maybe it's possible to account for all those cases with just a
> boolean... maybe not.

I wondered about this, too.

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

Vik,

* Vik Fearing (vik(dot)fearing(at)dalibo(dot)com) wrote:
> A parser refactoring broke my code. I reported it and it was promptly
> fixed. When the fix came up for review, I said it needed a regression
> test to prevent it from happening again and I was told by the author
> that such a test would be "flimsy" and it went on to be committed (by
> that same guy) without one. I'm undecided whether I'll be contributing
> there any further.

To be fair, we've declined to add regression tests, and in cases even
removed them, if they have been a source of false positives or add a lot
of extra time to the regression suite without really adding a lot of
coverage. We continue to discuss, and need imv, more regression tests,
grouped into different sets which are run depending on the environment
and local configuration. Running the regression suite while doing quick
code iterations should be fast, while the build farm should run as many
regression tests as practical (which might depend on the system).

There has been some progress on this front recently by Peter E and
Andrew Dunstan, as I recall, but I've not followed it very closely.
They may very well already have this set up.

> The rigor here makes me want to try and try again.

This is certainly good to hear and I hope that you continue to
contribute and keep on trying.

Thanks!

Stephen

On Thu, Oct 17, 2013 at 9:11 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 10/17/2013 02:42 PM, Robert Haas wrote:
>> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
>>> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
>>>> My guess is that it won't be committed if there is a single "but it
>>>> might break one code or surprise one user somewhere in the universe",
>>>> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
>>>> liner is really akin to "rejected".
>>> I have attached here an entirely new patch (new documentation and
>>> everything) that should please everyone. It no longer overloads
>>> pg_sleep(double precision) but instead add two new functions:
>>>
>>> * pg_sleep_for(interval)
>>> * pg_sleep_until(timestamp with time zone)
>>>
>>> Because it's no longer overloading the original pg_sleep, Robert's
>>> ambiguity objection is no more.
>>>
>>> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>>>
>>> If people like this, I'll reject the current patch and add this one to
>>> the next commitfest.
>> I find that naming relatively elegant. However, you've got to
>> schema-qualify every function and operator used in the definitions, or
>> you're creating a search-path security vulnerability.
>>
>
> Good catch. Updated patch attached.

Committed.

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

On 01/30/2014 09:48 PM, Robert Haas wrote:
> On Thu, Oct 17, 2013 at 9:11 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
>> On 10/17/2013 02:42 PM, Robert Haas wrote:
>>> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
>>>> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
>>>>> My guess is that it won't be committed if there is a single "but it
>>>>> might break one code or surprise one user somewhere in the universe",
>>>>> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
>>>>> liner is really akin to "rejected".
>>>> I have attached here an entirely new patch (new documentation and
>>>> everything) that should please everyone. It no longer overloads
>>>> pg_sleep(double precision) but instead add two new functions:
>>>>
>>>> * pg_sleep_for(interval)
>>>> * pg_sleep_until(timestamp with time zone)
>>>>
>>>> Because it's no longer overloading the original pg_sleep, Robert's
>>>> ambiguity objection is no more.
>>>>
>>>> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
>>>>
>>>> If people like this, I'll reject the current patch and add this one to
>>>> the next commitfest.
>>> I find that naming relatively elegant. However, you've got to
>>> schema-qualify every function and operator used in the definitions, or
>>> you're creating a search-path security vulnerability.
>>>
>> Good catch. Updated patch attached.
> Committed.

Thanks!

--
Vik

Hi

It seems like pg_sleep_until() has issues if used within a transaction, as
it uses now() and not clock_timestamp(). Please find attached a patch that
solves this issue.

For consistency reasons, I also modified pg_sleep_for() to also use
clock_timestamp.

Regards

On Fri, Jan 31, 2014 at 2:12 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:

> On 01/30/2014 09:48 PM, Robert Haas wrote:
> > On Thu, Oct 17, 2013 at 9:11 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
> wrote:
> >> On 10/17/2013 02:42 PM, Robert Haas wrote:
> >>> On Thu, Oct 17, 2013 at 8:26 AM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>
> wrote:
> >>>> On 10/17/2013 10:03 AM, Fabien COELHO wrote:
> >>>>> My guess is that it won't be committed if there is a single "but it
> >>>>> might break one code or surprise one user somewhere in the universe",
> >>>>> but I wish I'll be proven wrong. IMO, "returned with feedback" on a 1
> >>>>> liner is really akin to "rejected".
> >>>> I have attached here an entirely new patch (new documentation and
> >>>> everything) that should please everyone. It no longer overloads
> >>>> pg_sleep(double precision) but instead add two new functions:
> >>>>
> >>>> * pg_sleep_for(interval)
> >>>> * pg_sleep_until(timestamp with time zone)
> >>>>
> >>>> Because it's no longer overloading the original pg_sleep, Robert's
> >>>> ambiguity objection is no more.
> >>>>
> >>>> Also, I like how it reads aloud: SELECT pg_sleep_for('5 minutes');
> >>>>
> >>>> If people like this, I'll reject the current patch and add this one to
> >>>> the next commitfest.
> >>> I find that naming relatively elegant. However, you've got to
> >>> schema-qualify every function and operator used in the definitions, or
> >>> you're creating a search-path security vulnerability.
> >>>
> >> Good catch. Updated patch attached.
> > Committed.
>
> Thanks!
>
> --
> Vik
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Sun, Feb 2, 2014 at 4:50 AM, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> It seems like pg_sleep_until() has issues if used within a transaction, as
> it uses now() and not clock_timestamp(). Please find attached a patch that
> solves this issue.
>
> For consistency reasons, I also modified pg_sleep_for() to also use
> clock_timestamp.

Good catch!

Committed.

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