Extra check in 9.0 exclusion constraint unintended consequences

Hi:

On Tue, Jul 5, 2011 at 11:26 AM, Jeff Davis <pgsql@j-davis.com> wrote:

In the 9.0 version of exclusion constraints, we added an extra check to
ensure that, when searching for a conflict, a tuple at least found
itself as a conflict. This extra check is not present in 9.1+.

It was designed to help diagnose certain types of problems, and is fine
for most use cases. A value is equal to itself (and therefore conflicts
with itself), and a value overlaps with itself (and therefore conflicts
with itself), which were the primary use cases. We removed the extra
check in 9.1 because there are other operators for which that might not
be true, like <>, but the use case is a little more obscure.

However, values don't always overlap with themselves -- for instance the
empty period (which was an oversight by me). So, Abel Abraham Camarillo
Ojeda ran into a rather cryptic error message when he tried to do that:

ERROR:  failed to re-find tuple within index "t_period_excl"
HINT:  This may be because of a non-immutable index expression.

I don't think we need to necessarily remove the extra check in 9.0,
because the workaround is simple: add a WHERE clause to the constraint
eliminating empty periods. Perhaps we could improve the error message
and hint, and add a note in the documentation.

That's what I'm doing now: using a where clause to workaround... it's easy, but
I was still amazed about what that error message meant...

Thanks.

On Tue, Jul 5, 2011 at 12:26 PM, Jeff Davis <pgsql@j-davis.com> wrote:

In the 9.0 version of exclusion constraints, we added an extra check to
ensure that, when searching for a conflict, a tuple at least found
itself as a conflict. This extra check is not present in 9.1+.

It was designed to help diagnose certain types of problems, and is fine
for most use cases. A value is equal to itself (and therefore conflicts
with itself), and a value overlaps with itself (and therefore conflicts
with itself), which were the primary use cases. We removed the extra
check in 9.1 because there are other operators for which that might not
be true, like <>, but the use case is a little more obscure.

However, values don't always overlap with themselves -- for instance the
empty period (which was an oversight by me). So, Abel Abraham Camarillo
Ojeda ran into a rather cryptic error message when he tried to do that:

ERROR:  failed to re-find tuple within index "t_period_excl"
HINT:  This may be because of a non-immutable index expression.

I don't think we need to necessarily remove the extra check in 9.0,
because the workaround is simple: add a WHERE clause to the constraint
eliminating empty periods. Perhaps we could improve the error message
and hint, and add a note in the documentation.

I think it's probably too late to go fiddling with the behavior of 9.0
at this point. If we change the text of error messages, there is a
chance that it might break applications; it would also require those
messages to be re-translated, and I don't think the issue is really
important enough to justify a change. I am happy to see us document
it better, though, since it's pretty clear that there is more
likelihood of hitting that error than we might have suspected at the
outset.

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

On Thu, 2011-07-07 at 12:36 -0400, Robert Haas wrote:

I think it's probably too late to go fiddling with the behavior of 9.0
at this point. If we change the text of error messages, there is a
chance that it might break applications; it would also require those
messages to be re-translated, and I don't think the issue is really
important enough to justify a change.

Good point on the error messages -- I didn't really think of that as a
big deal.

I am happy to see us document
it better, though, since it's pretty clear that there is more
likelihood of hitting that error than we might have suspected at the
outset.

Doc patch attached, but I'm not attached to the wording. Remember that
we only need to update the 9.0 docs, I don't think you want to apply
this to master (though I'm not sure how this kind of thing is normally
handled).

Regards,
Jeff Davis

On Fri, Jul 8, 2011 at 12:58 AM, Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2011-07-07 at 12:36 -0400, Robert Haas wrote:

I think it's probably too late to go fiddling with the behavior of 9.0
at this point.  If we change the text of error messages, there is a
chance that it might break applications; it would also require those
messages to be re-translated, and I don't think the issue is really
important enough to justify a change.

Good point on the error messages -- I didn't really think of that as a
big deal.

I am happy to see us document
it better, though, since it's pretty clear that there is more
likelihood of hitting that error than we might have suspected at the
outset.

Doc patch attached, but I'm not attached to the wording. Remember that
we only need to update the 9.0 docs, I don't think you want to apply
this to master (though I'm not sure how this kind of thing is normally
handled).

I'm wondering if we might want to call this out with a <note> or
similar... especially if we're only going to put it into the 9.0
docs.

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

On Fri, 2011-07-08 at 22:51 -0400, Robert Haas wrote:

I'm wondering if we might want to call this out with a <note> or
similar... especially if we're only going to put it into the 9.0
docs.

Sure, sounds good.

Regards,
Jeff Davis

Excerpts from Jeff Davis's message of vie jul 08 00:58:20 -0400 2011:

On Thu, 2011-07-07 at 12:36 -0400, Robert Haas wrote:

I think it's probably too late to go fiddling with the behavior of 9.0
at this point. If we change the text of error messages, there is a
chance that it might break applications; it would also require those
messages to be re-translated, and I don't think the issue is really
important enough to justify a change.

Good point on the error messages -- I didn't really think of that as a
big deal.

I am happy to see us document
it better, though, since it's pretty clear that there is more
likelihood of hitting that error than we might have suspected at the
outset.

Doc patch attached, but I'm not attached to the wording. Remember that
we only need to update the 9.0 docs, I don't think you want to apply
this to master (though I'm not sure how this kind of thing is normally
handled).

Is this really a good idea? I think the note should still be there in
9.1 and beyond (with the version applicability note of course)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Sun, 2011-07-10 at 00:36 -0400, Alvaro Herrera wrote:

Is this really a good idea? I think the note should still be there in
9.1 and beyond (with the version applicability note of course)

I see your point, but it also seems strange to keep such a note
permanently. And it also seems minor enough that we don't want it to be
another thing to keep track of.

I don't really have a strong opinion here. People might hit in in 9.0,
but there's a workaround. And they won't hit it in 9.1+.

Regards,
Jeff Davis

On Sun, Jul 10, 2011 at 3:29 PM, Jeff Davis <pgsql@j-davis.com> wrote:

On Sun, 2011-07-10 at 00:36 -0400, Alvaro Herrera wrote:

Is this really a good idea?  I think the note should still be there in
9.1 and beyond (with the version applicability note of course)

I see your point, but it also seems strange to keep such a note
permanently. And it also seems minor enough that we don't want it to be
another thing to keep track of.

I don't really have a strong opinion here. People might hit in in 9.0,
but there's a workaround. And they won't hit it in 9.1+.

I dropped the ball on this, mostly because I was on vacation the week
we were having this discussion - and by the time I got back it was too
far down in the folder.

I'm OK with adding a note either to the 9.0 docs only (which means it
might be missed by a 9.0 user who only looks at the current docs) or
with adding a note to all versions mentioning the difference in
behavior with 9.0, but I'm not really sure which way to go with it.
Or we could just not do anything at all. Anyone else have an opinion?

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

On Thu, 2011-08-11 at 11:58 -0400, Robert Haas wrote:

I'm OK with adding a note either to the 9.0 docs only (which means it
might be missed by a 9.0 user who only looks at the current docs) or
with adding a note to all versions mentioning the difference in
behavior with 9.0, but I'm not really sure which way to go with it.
Or we could just not do anything at all. Anyone else have an opinion?

It seems to be somewhat of a burden to carry a version-specific note
indefinitely... more clutter than helpful. So I'd vote for just changing
the 9.0 docs.

Or, we could add the 9.0-specific note to 9.0 and 9.1 docs, but leave it
out of 'master'. That way it sticks around for a while but we don't have
to remember to remove it later.

Regards,
Jeff Davis

On Thu, Aug 11, 2011 at 2:25 PM, Jeff Davis <pgsql@j-davis.com> wrote:

On Thu, 2011-08-11 at 11:58 -0400, Robert Haas wrote:

I'm OK with adding a note either to the 9.0 docs only (which means it
might be missed by a 9.0 user who only looks at the current docs) or
with adding a note to all versions mentioning the difference in
behavior with 9.0, but I'm not really sure which way to go with it.
Or we could just not do anything at all.  Anyone else have an opinion?

It seems to be somewhat of a burden to carry a version-specific note
indefinitely... more clutter than helpful. So I'd vote for just changing
the 9.0 docs.

Or, we could add the 9.0-specific note to 9.0 and 9.1 docs, but leave it
out of 'master'. That way it sticks around for a while but we don't have
to remember to remove it later.

Having thought about this a bit further, I'm coming around to the view
that if it isn't worth adding this in master, it's not worth adding at
all. I just don't think it's going to get any visibility as a
back-branch only doc patch.

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

On Fri, 2011-08-12 at 14:58 -0400, Robert Haas wrote:

Having thought about this a bit further, I'm coming around to the view
that if it isn't worth adding this in master, it's not worth adding at
all. I just don't think it's going to get any visibility as a
back-branch only doc patch.

Fine with me.

Regards,
Jeff Davis