Negative LIMIT and OFFSET?

"Simon Riggs" <simon@2ndquadrant.com> writes:

I'm fairly surprised these queries work. Is there some reason why we
support this? April Fools Day? Jules Verne? I'm all for fast queries,
but zero seems like the lowest value we should support...

Huh, I was all set to post an example of a useful application of it but then
apparently I'm wrong and it doesn't work:

postgres=# select * from generate_series(1,10) offset -1 limit 2;
generate_series
-----------------
1
2
(2 rows)

I'll leave it as an exercise for the reader to guess what I was expecting.

So given that that doesn't work I don't see any particular reason to accept
negative offsets or limits in 8.4 and on.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

On Fri, Dec 14, 2007 at 01:47:23AM +0000, Gregory Stark wrote:

Huh, I was all set to post an example of a useful application of it but then
apparently I'm wrong and it doesn't work:

I dimly remember some discussion of this issue once before, maybe a year
ago. My memory isn't what it was, and I can't find it by trolling archives,
but I recall Tom saying that it was dumb, yes, but don't do that, because
there's some reason not to change it. I know, helpful search terms R me.

A

On Dec 13, 2007 9:43 PM, Andrew Sullivan <ajs@crankycanuck.ca> wrote:

I dimly remember some discussion of this issue once before, maybe a year
ago. My memory isn't what it was, and I can't find it by trolling archives,
but I recall Tom saying that it was dumb, yes, but don't do that, because
there's some reason not to change it. I know, helpful search terms R me.

Man, maybe my mad Google skillz are not as mad as I thought :(

The best I could come up with was on my first try, though as it's just
a reply to a user asking for a different behavior of it, I doubt this
is it:

http://archives.postgresql.org/pgsql-general/2002-10/msg01293.php

Google search terms:

site:archives.postgresql.org "tom lane" "limit -1" "negative"

While I haven't looked at the code myself, I tend to agree with Simon
and Greg... I know of no reason to allow a negative limit/offset.

--
Jonah H. Harris, Sr. Software Architect | phone: 732.331.1324
EnterpriseDB Corporation | fax: 732.331.1301
499 Thornall Street, 2nd Floor | jonah.harris@enterprisedb.com
Edison, NJ 08837 | http://www.enterprisedb.com/

Andrew Sullivan <ajs@crankycanuck.ca> writes:

On Fri, Dec 14, 2007 at 01:47:23AM +0000, Gregory Stark wrote:

Huh, I was all set to post an example of a useful application of it but then
apparently I'm wrong and it doesn't work:

I dimly remember some discussion of this issue once before, maybe a year
ago. My memory isn't what it was, and I can't find it by trolling archives,
but I recall Tom saying that it was dumb, yes, but don't do that, because
there's some reason not to change it. I know, helpful search terms R me.

Hmm ... I don't recall much either. The code in nodeLimit.c just
silently replaces a negative input value by zero. It'd certainly be
possible to make it throw an error instead, but what the downsides of
that might be aren't clear.

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec. If you want to throw an
error for negative limit, shouldn't you logically also throw an error
for limit larger than the actual number of rows produced by the subplan?

regards, tom lane

On Thu, Dec 13, 2007 at 10:01:43PM -0500, Jonah H. Harris wrote:

Man, maybe my mad Google skillz are not as mad as I thought :(

Hey, I worked in a library some years ago, when Google was just a googlet,
and I couldn't find it either. It's a dim memory, note. Which could mean
"artifact". I'm old! I'm probably delusional too.

A

On Dec 13, 2007 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec. If you want to throw an
error for negative limit, shouldn't you logically also throw an error
for limit larger than the actual number of rows produced by the subplan?

Hmm, good point. It does seem like if you're going to be pedantic,
you should be pedantic on both counts.

Though, I could understand throwing an error on a negative, because
that's likely a bug in the user's code and would enable them to find
out what's wrong. On the limit-larger-than-tuples-returned case, I
don't think it should throw an error because it's generally considered
as, "at most this many". I don't see a case where any user would
think that a negative limit *should* be allowed.

Don't we have any similar usability cases in the system like this,
where negatives are not allowed only for the sake of it being an
insane setting? I'm tired, but I thought we did.

--
Jonah H. Harris, Sr. Software Architect | phone: 732.331.1324
EnterpriseDB Corporation | fax: 732.331.1301
499 Thornall Street, 2nd Floor | jonah.harris@enterprisedb.com
Edison, NJ 08837 | http://www.enterprisedb.com/

On Thu, Dec 13, 2007 at 10:06:35PM -0500, Tom Lane wrote:

of a negative limit, it's meeting its spec. If you want to throw an
error for negative limit, shouldn't you logically also throw an error

Should it be a WARNING?

A

"Jonah H. Harris" <jonah.harris@gmail.com> writes:

Don't we have any similar usability cases in the system like this,
where negatives are not allowed only for the sake of it being an
insane setting? I'm tired, but I thought we did.

Yeah, probably. It's the kind of thing where the call is close enough
that it might be made differently by different people.

After thinking about it for a bit, the only downside I can think of is
that throwing an error might create an unexpected corner case for code
that computes a LIMIT value on-the-fly and might sometimes come up
with a slightly negative value. But you could always do
LIMIT greatest(whatever, 0)
so that seems like a weak argument.

regards, tom lane

On Dec 13, 2007 10:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Sullivan <ajs@crankycanuck.ca> writes:

On Fri, Dec 14, 2007 at 01:47:23AM +0000, Gregory Stark wrote:

Huh, I was all set to post an example of a useful application of it but then
apparently I'm wrong and it doesn't work:

I dimly remember some discussion of this issue once before, maybe a year
ago. My memory isn't what it was, and I can't find it by trolling archives,
but I recall Tom saying that it was dumb, yes, but don't do that, because
there's some reason not to change it. I know, helpful search terms R me.

Hmm ... I don't recall much either. The code in nodeLimit.c just
silently replaces a negative input value by zero. It'd certainly be
possible to make it throw an error instead, but what the downsides of
that might be aren't clear.

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec. If you want to throw an
error for negative limit, shouldn't you logically also throw an error
for limit larger than the actual number of rows produced by the subplan?

for historical record, this comment (subject not directly related to
the OP) was probably this:
http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg62562.html

at least if it happened since 10/2004, which is when i started
tracking -hackers in my gmail account (an amazing search tool, btw).

merlin

On Thu, 2007-12-13 at 22:06 -0500, Tom Lane wrote:

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec.

If "LIMIT n" means "emit at most n tuples", then a query that produces 0
rows with n < 0 is arguably violating its spec, since it has produced
more tuples than the LIMIT specified (0 > n). Interpreted this way, no
result set can be consistent with a negative limit, so I'd vote for
throwing an error.

-Neil

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

"Jonah H. Harris" <jonah.harris@gmail.com> writes:

Don't we have any similar usability cases in the system like this,
where negatives are not allowed only for the sake of it being an
insane setting? I'm tired, but I thought we did.

Yeah, probably. It's the kind of thing where the call is close enough
that it might be made differently by different people.

After thinking about it for a bit, the only downside I can think of is
that throwing an error might create an unexpected corner case for code
that computes a LIMIT value on-the-fly and might sometimes come up
with a slightly negative value.

See, that's what I was thinking when I wrote the OFFSET -1 LIMIT 2 test. If
that produced 1 record then I would say it avoids a corner case for user code
which would have to special case windows which hit the beginning or end of the
data set. But given that it doesn't work that way anyways user code already
has a big corner case.

The argument for errors that I see is: Having a useless non-error behaviour
locks us into keeping that. If we one day think of a useful set of semantics
we can't just silently change the meaning on users without a transition period
of generating errors.

For example we might want to implement negative offsets like above, or if we
want to implement negative limits as meaning some number of rows *before* the
offset or end of relation, or something like that...

The flip side of all this is that it's hard to get too excited about it. It's
just OFFSET / LIMIT. If it was a big deal we would have noticed years ago
anyways. It might not be worth the effort to change and introduce behaviour
changes for users at all.

Oh, and incidentally the problem with WARNING is that this is DML which could
potentially be executing hundreds or thousands of times per minute. A WARNING
is effectively an ERROR.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's Slony Replication support!

On Thu, 2007-12-13 at 22:06 -0500, Tom Lane wrote:

Hmm ... I don't recall much either. The code in nodeLimit.c just
silently replaces a negative input value by zero. It'd certainly be
possible to make it throw an error instead, but what the downsides of
that might be aren't clear.

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec.

Not bothered either way, really, just reporting weirdness as it comes.

A calculated LIMIT value that was negative probably indicates an error
in the calculation that the SQL programmer may wish to know about. I had
previously assumed that we would report such errors.

If you want to throw an
error for negative limit, shouldn't you logically also throw an error
for limit larger than the actual number of rows produced by the subplan?

No, thats another feature AT LEAST n, which could also be a useful
thing, like an Assert.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On Thu, 2007-12-13 at 22:23 -0800, Neil Conway wrote:

On Thu, 2007-12-13 at 22:06 -0500, Tom Lane wrote:

I guess that on purely philosophical grounds, it's not an unreasonable
behavior. For example, "LIMIT n" means "output at most n tuples",
not "output exactly n tuples". So when it outputs no tuples in the face
of a negative limit, it's meeting its spec.

If "LIMIT n" means "emit at most n tuples", then a query that produces 0
rows with n < 0 is arguably violating its spec, since it has produced
more tuples than the LIMIT specified (0 > n). Interpreted this way, no
result set can be consistent with a negative limit, so I'd vote for
throwing an error.

I even found an existing, unused error message called
ERRCODE_INVALID_LIMIT_VALUE

so here's a patch.

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On Fri, 2007-12-14 at 14:41 +0000, Simon Riggs wrote:

On Thu, 2007-12-13 at 22:23 -0800, Neil Conway wrote:

so here's a patch.

minor correction

--
Simon Riggs
2ndQuadrant http://www.2ndQuadrant.com

On Fri, Dec 14, 2007 at 09:02:04AM +0000, Gregory Stark wrote:

Oh, and incidentally the problem with WARNING is that this is DML which could
potentially be executing hundreds or thousands of times per minute. A WARNING
is effectively an ERROR.

Good point. Also, the sort of case where you're likely to be automatically
generating these negative values is also the sort of case where you have
various nice programmatic interfaces, many of which store up all the
warnings. The warnings then have to be freed explicitly, which of course
means that by adding a warning, clients would suddenly start to chew through
piles of memory.

A

On Thu, Dec 13, 2007 at 11:31:17PM -0500, Merlin Moncure wrote:

for historical record, this comment (subject not directly related to
the OP) was probably this:
http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg62562.html

Bingo. Thanks!

A

Simon Riggs <simon@2ndquadrant.com> writes:

I even found an existing, unused error message called
ERRCODE_INVALID_LIMIT_VALUE

That's a bad idea I think. That code is defined by SQL99. I can't find
anyplace that they specify what it should be raised for, but we can be
pretty confident that it's not meant for LIMIT. I think we should
just use INVALID_PARAMETER_VALUE.

How do people feel about applying this to 8.3, rather than holding it?
One possible objection is that we're past string freeze, but I noted
Peter doing some message editorializing as recently as today, so it
would seem a slushy freeze at best.

regards, tom lane

On Dec 14, 2007 6:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

How do people feel about applying this to 8.3, rather than holding it?
One possible objection is that we're past string freeze, but I noted
Peter doing some message editorializing as recently as today, so it
would seem a slushy freeze at best.

FWIW, I'm good with applying it to 8.3.

--
Jonah H. Harris, Sr. Software Architect | phone: 732.331.1324
EnterpriseDB Corporation | fax: 732.331.1301
499 Thornall Street, 2nd Floor | jonah.harris@enterprisedb.com
Edison, NJ 08837 | http://www.enterprisedb.com/

"Jonah H. Harris" <jonah.harris@gmail.com> writes:

On Dec 14, 2007 6:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

How do people feel about applying this to 8.3, rather than holding it?
One possible objection is that we're past string freeze, but I noted
Peter doing some message editorializing as recently as today, so it
would seem a slushy freeze at best.

FWIW, I'm good with applying it to 8.3.

I think it would have been better to apply before beta. We would have found
out if users were going to complain about it. Perhaps we should do it for 8.4
instead

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!