Re: [HACKERS] strange IS NULL behaviour
On Mon, Sep 9, 2013 at 3:51 PM, Bruce Momjian br...@momjian.us wrote: The problem is that I don't believe this patch is commit-ready --- someone needs to research the IS NULL tests in all areas of our code to see if they match this patch, and I can't do that. Is that something a reviewer is going to be willing to do? I don't think I have ever seen a commit-fest item that still required serious research outside the patch area before committing. I could ask just for feedback, but I have already received enough feedback to know I can't get the patch to a ready-enough state. OK, well then there's probably not much point. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 08:45:14AM -0400, Robert Haas wrote: On Mon, Sep 9, 2013 at 3:51 PM, Bruce Momjian br...@momjian.us wrote: The problem is that I don't believe this patch is commit-ready --- someone needs to research the IS NULL tests in all areas of our code to see if they match this patch, and I can't do that. Is that something a reviewer is going to be willing to do? I don't think I have ever seen a commit-fest item that still required serious research outside the patch area before committing. I could ask just for feedback, but I have already received enough feedback to know I can't get the patch to a ready-enough state. OK, well then there's probably not much point. FYI, I think these queries below prove that NOT NULL constraints do not follow the single-depth ROW NULL inspection rule that PL/pgSQL follows, and that my patch was trying to promote for queries: CREATE TABLE test2(x test NOT NULL); CREATE TABLE INSERT INTO test2 VALUES (null); ERROR: null value in column x violates not-null constraint DETAIL: Failing row contains (null). -- INSERT INTO test2 VALUES (row(null)); INSERT 0 1 So, in summary, NOT NULL constraints don't inspect into ROW values for NULLs, PL/pgSQL goes one level deep into ROW, and queries go two levels deep. I am not sure what other areas need checking. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 8:56 AM, Bruce Momjian br...@momjian.us wrote:
On Tue, Sep 10, 2013 at 08:45:14AM -0400, Robert Haas wrote:
On Mon, Sep 9, 2013 at 3:51 PM, Bruce Momjian br...@momjian.us wrote:
The problem is that I don't believe this patch is commit-ready ---
someone needs to research the IS NULL tests in all areas of our code to
see if they match this patch, and I can't do that. Is that something a
reviewer is going to be willing to do? I don't think I have ever seen a
commit-fest item that still required serious research outside the patch
area before committing. I could ask just for feedback, but I have
already received enough feedback to know I can't get the patch to a
ready-enough state.
OK, well then there's probably not much point.
FYI, I think these queries below prove that NOT NULL constraints do not
follow the single-depth ROW NULL inspection rule that PL/pgSQL follows,
and that my patch was trying to promote for queries:
CREATE TABLE test2(x test NOT NULL);
CREATE TABLE
INSERT INTO test2 VALUES (null);
ERROR: null value in column x violates not-null constraint
DETAIL: Failing row contains (null).
-- INSERT INTO test2 VALUES (row(null));
INSERT 0 1
So, in summary, NOT NULL constraints don't inspect into ROW values for
NULLs, PL/pgSQL goes one level deep into ROW, and queries go two levels
deep. I am not sure what other areas need checking.
Our composite null handling (as noted) is an absolute minefield of
issues. Consider:
postgres=# select coalesce(row(null,null), row('no', 'bueno'));
coalesce
--
(,)
postgres=# select case when row(null,null) is null then row('no', 'bueno') end;
case
(no,bueno)
It's just a mess. So it bears repeating: do we or do we not want to
implement SQL standard composite null handing? If so, you probably
have to hit all the targets. If not, I'd either A: leave things alone
or B: remove the special case logic in IS NULL (so that it behaves as
coalesce() does) and document our divergence from the standard. Point
being: B might actually be the best choice, but it should be
understood that we are not going in that direction before pushing
patches that go in the other direction.
merlin
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 09:12:08AM -0500, Merlin Moncure wrote:
FYI, I think these queries below prove that NOT NULL constraints do not
follow the single-depth ROW NULL inspection rule that PL/pgSQL follows,
and that my patch was trying to promote for queries:
CREATE TABLE test2(x test NOT NULL);
CREATE TABLE
INSERT INTO test2 VALUES (null);
ERROR: null value in column x violates not-null constraint
DETAIL: Failing row contains (null).
-- INSERT INTO test2 VALUES (row(null));
INSERT 0 1
So, in summary, NOT NULL constraints don't inspect into ROW values for
NULLs, PL/pgSQL goes one level deep into ROW, and queries go two levels
deep. I am not sure what other areas need checking.
Our composite null handling (as noted) is an absolute minefield of
issues. Consider:
postgres=# select coalesce(row(null,null), row('no', 'bueno'));
coalesce
--
(,)
postgres=# select case when row(null,null) is null then row('no', 'bueno')
end;
case
(no,bueno)
It's just a mess. So it bears repeating: do we or do we not want to
Wow, OK, more inconsistent places. :-(
implement SQL standard composite null handing? If so, you probably
I am unclear if section 8.7, Null Predicate, in the SQL 2003 standard is
talking about just IS NULL, or all NULL tests.
have to hit all the targets. If not, I'd either A: leave things alone
or B: remove the special case logic in IS NULL (so that it behaves as
coalesce() does) and document our divergence from the standard. Point
being: B might actually be the best choice, but it should be
understood that we are not going in that direction before pushing
patches that go in the other direction.
I see. So going one-level deep in the ROW NULL inspection is something
we do for IS NULL in queries (actually double-deep inspection)q, but it
was never consistently implemented across all NULL tests.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 10:50:32AM -0400, Bruce Momjian wrote:
have to hit all the targets. If not, I'd either A: leave things alone
or B: remove the special case logic in IS NULL (so that it behaves as
coalesce() does) and document our divergence from the standard. Point
being: B might actually be the best choice, but it should be
understood that we are not going in that direction before pushing
patches that go in the other direction.
I see. So going one-level deep in the ROW NULL inspection is something
we do for IS NULL in queries (actually double-deep inspection)q, but it
was never consistently implemented across all NULL tests.
Using your examples and others I have collected, I have created an SQL
script which shows our inconsistent behavior, attached, and its output.
If we agree that a single-level NULL inspection of ROWS is the right
approach, it would seem we need my patch, and we need to fix coalesce()
and NOT NULL constraint testing? Is that accurate? Is there more
areas?
Nested RECORDS seem to collapse to a single level, so I don't think we
have to change the recursion there:
SELECT RECORD(RECORD(RECORD(NULL)));
record
(null)
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
SELECT ROW(NULL) IS NULL;
SELECT ROW(ROW(NULL)) IS NULL;
SELECT ROW(ROW(ROW(NULL))) IS NULL;
SELECT RECORD(NULL) IS NULL;
SELECT RECORD(RECORD(NULL)) IS NULL;
SELECT RECORD(RECORD(RECORD(NULL))) IS NULL;
DROP TABLE IF EXISTS tt;
CREATE TABLE tt (x INT);
INSERT INTO tt VALUES(NULL);
SELECT ROW(x) IS NULL FROM tt;
SELECT ROW(ROW(x)) IS NULL FROM tt;
SELECT ROW(ROW(ROW(x))) IS NULL FROM tt;
SELECT coalesce(ROW(NULL,NULL), ROW('no', 'bueno'));
SELECT CASE WHEN ROW(NULL,NULL) IS NULL THEN ROW('no', 'bueno') END;
DROP TABLE IF EXISTS test3;
DROP TABLE IF EXISTS test2;
CREATE TABLE test2 (x INT);
CREATE TABLE test3(x test2 NOT NULL);
INSERT INTO test3 VALUES (null);
INSERT INTO test3 VALUES (row(null));
DO LANGUAGE plpgsql $$
DECLARE
r RECORD;
BEGIN
DROP TABLE IF EXISTS test;
CREATE TABLE test (x INT, y INT);
INSERT INTO test VALUES (1, NULL), (NULL, 1), (NULL, NULL);
FOR r IN SELECT * FROM test
LOOP
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
END LOOP;
END;
$$;
DO LANGUAGE plpgsql $$
DECLARE
r RECORD;
BEGIN
SELECT NULL INTO r;
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
SELECT ROW(NULL) INTO r;
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
END;
$$;
SELECT ROW(NULL) IS NULL;
?column?
--
t
(1 row)
SELECT ROW(ROW(NULL)) IS NULL;
?column?
--
t
(1 row)
SELECT ROW(ROW(ROW(NULL))) IS NULL;
?column?
--
f
(1 row)
SELECT RECORD(NULL) IS NULL;
?column?
--
t
(1 row)
SELECT RECORD(RECORD(NULL)) IS NULL;
?column?
--
t
(1 row)
SELECT RECORD(RECORD(RECORD(NULL))) IS NULL;
?column?
--
t
(1 row)
DROP TABLE IF EXISTS tt;
DROP TABLE
CREATE TABLE tt (x INT);
CREATE TABLE
INSERT INTO tt VALUES(NULL);
INSERT 0 1
SELECT ROW(x) IS NULL FROM tt;
?column?
--
t
(1 row)
SELECT ROW(ROW(x)) IS NULL FROM tt;
?column?
--
t
(1 row)
SELECT ROW(ROW(ROW(x))) IS NULL FROM tt;
?column?
--
f
(1 row)
SELECT coalesce(ROW(NULL,NULL), ROW('no', 'bueno'));
coalesce
--
(,)
(1 row)
SELECT CASE WHEN ROW(NULL,NULL) IS NULL THEN ROW('no', 'bueno') END;
case
(no,bueno)
(1 row)
DROP TABLE IF EXISTS test3;
DROP TABLE
DROP TABLE IF EXISTS test2;
DROP TABLE
CREATE TABLE test2 (x INT);
CREATE TABLE
CREATE TABLE test3(x test2 NOT NULL);
CREATE TABLE
INSERT INTO test3 VALUES (null);
ERROR: null value in column x violates not-null constraint
DETAIL: Failing row contains (null).
INSERT INTO test3 VALUES (row(null));
INSERT 0 1
DO LANGUAGE plpgsql $$
DECLARE
r RECORD;
BEGIN
DROP TABLE IF EXISTS test;
CREATE TABLE test (x INT, y INT);
INSERT INTO test VALUES (1, NULL), (NULL, 1), (NULL, NULL);
FOR r IN SELECT * FROM test
LOOP
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
END LOOP;
END;
$$;
NOTICE: false
NOTICE: false
NOTICE: true
DO
DO LANGUAGE plpgsql $$
DECLARE
r RECORD;
BEGIN
SELECT NULL INTO r;
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
SELECT ROW(NULL) INTO r;
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
END;
$$;
NOTICE: true
NOTICE: false
DO
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us wrote: Is IS DISTINCT FROM correct though? SELECT ROW(NULL) IS DISTINCT FROM NULL; ?column? -- t (1 row) My recollection from previous discussions is that this is what is required by the standard. ROW(NULL) IS NULL, but it is DISTINCT FROM NULL. The IS NULL predicate, when applied to a row or record is meant to indicate whether that row or record *contains only NULL elements*, and IS NOT NULL is meant to indicate that a row or record *contains only NOT NULL elements*. So this is all as required: test=# create table x (c1 int, c2 int); CREATE TABLE test=# insert into x values (1, 1), (2, null), (null, 3), (null, null); INSERT 0 4 test=# select * from x where x is not null; c1 | c2 + 1 | 1 (1 row) test=# select * from x where x is null; c1 | c2 + | (1 row) test=# select * from x where not x is null; c1 | c2 + 1 | 1 2 | | 3 (3 rows) test=# select * from x where not x is not null; c1 | c2 + 2 | | 3 | (3 rows) -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 12:48:08PM -0700, Kevin Grittner wrote: Bruce Momjian br...@momjian.us wrote: FYI, I think these queries below prove that NOT NULL constraints do not follow the single-depth ROW NULL inspection rule that PL/pgSQL follows, and that my patch was trying to promote for queries: CREATE TABLE test2(x test NOT NULL); CREATE TABLE INSERT INTO test2 VALUES (null); ERROR: null value in column x violates not-null constraint DETAIL: Failing row contains (null). -- INSERT INTO test2 VALUES (row(null)); INSERT 0 1 If I remember correctly, the standard wants a NOT NULL constraint on a column with a composite type to behave the same as CHECK (col IS DISTINCT FROM NULL) ... which is consistent with the behavior you show. Is IS DISTINCT FROM correct though? SELECT ROW(NULL) IS DISTINCT FROM NULL; ?column? -- t (1 row) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us wrote: FYI, I think these queries below prove that NOT NULL constraints do not follow the single-depth ROW NULL inspection rule that PL/pgSQL follows, and that my patch was trying to promote for queries: CREATE TABLE test2(x test NOT NULL); CREATE TABLE INSERT INTO test2 VALUES (null); ERROR: null value in column x violates not-null constraint DETAIL: Failing row contains (null). -- INSERT INTO test2 VALUES (row(null)); INSERT 0 1 If I remember correctly, the standard wants a NOT NULL constraint on a column with a composite type to behave the same as CHECK (col IS DISTINCT FROM NULL) ... which is consistent with the behavior you show. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 10, 2013 at 1:54 PM, Bruce Momjian br...@momjian.us wrote: On Tue, Sep 10, 2013 at 10:50:32AM -0400, Bruce Momjian wrote: have to hit all the targets. If not, I'd either A: leave things alone or B: remove the special case logic in IS NULL (so that it behaves as coalesce() does) and document our divergence from the standard. Point being: B might actually be the best choice, but it should be understood that we are not going in that direction before pushing patches that go in the other direction. I see. So going one-level deep in the ROW NULL inspection is something we do for IS NULL in queries (actually double-deep inspection)q, but it was never consistently implemented across all NULL tests. Using your examples and others I have collected, I have created an SQL script which shows our inconsistent behavior, attached, and its output. If we agree that a single-level NULL inspection of ROWS is the right approach, it would seem we need my patch, and we need to fix coalesce() and NOT NULL constraint testing? Is that accurate? Is there more areas? Nested RECORDS seem to collapse to a single level, so I don't think we have to change the recursion there: SELECT RECORD(RECORD(RECORD(NULL))); record (null) Also consider: STRICT PQisNull EXCEPT/UNION WHERE NOT IN (see how it plays with 'null gotcha') etc (but I don't think your lens should be focused on 'record recursion' -- there is a deeper problem for which that is just a particular symptom) merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Sat, Sep 7, 2013 at 10:59 AM, Bruce Momjian br...@momjian.us wrote: Why don't you add the proposal to the commitfest? This issue is so much larger than the patch's validity that I don't see how that would work. I hate to be rude here, but I think you're being ridiculous. We have a well-established procedure for getting patches reviewed around here, and while it is not perfect, it mostly works. If you try that procedure and it doesn't work, then I think you have a right to complain. But to object, on the one hand, that people aren't going to look at the patch, and then to refuse to add it to the tracking tool that the project uses to ensure that patches get looked at, seems patently unfair. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Mon, Sep 9, 2013 at 12:37:25PM -0400, Robert Haas wrote: On Sat, Sep 7, 2013 at 10:59 AM, Bruce Momjian br...@momjian.us wrote: Why don't you add the proposal to the commitfest? This issue is so much larger than the patch's validity that I don't see how that would work. I hate to be rude here, but I think you're being ridiculous. We have a well-established procedure for getting patches reviewed around here, and while it is not perfect, it mostly works. If you try that procedure and it doesn't work, then I think you have a right to complain. But to object, on the one hand, that people aren't going to look at the patch, and then to refuse to add it to the tracking tool that the project uses to ensure that patches get looked at, seems patently unfair. The problem is that I don't believe this patch is commit-ready --- someone needs to research the IS NULL tests in all areas of our code to see if they match this patch, and I can't do that. Is that something a reviewer is going to be willing to do? I don't think I have ever seen a commit-fest item that still required serious research outside the patch area before committing. I could ask just for feedback, but I have already received enough feedback to know I can't get the patch to a ready-enough state. I think requiring commit-fest reviewers come to the same conclusion is just making extra work for them. Still, if you want me to add it to the next commit-fest, please let me know. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Sat, Sep 7, 2013 at 07:42:55AM +0200, Andres Freund wrote: On 2013-09-06 23:07:04 -0400, Bruce Momjian wrote: On Fri, Sep 6, 2013 at 11:00:24PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Sep 5, 2013 at 05:06:41PM -0400, Bruce Momjian wrote: Another possible fix would be to avoid the IS NULL value optimizer expansion if a ROW construct is inside a ROW(). I have attached a patch that does this for review. Having received no replies, do people perfer this version of the patch that just punts nested ROW IS NULL testing to execQual.c? For some reason I read your previous message as saying you were willing to wait for considered reviews this time. If not, I'll just write a blanket -1 for any version of this patch. Are you saying people will comment later? I wasn't clear that was the plan. I can certainly wait. You do realize mere mortals in the project frequently have to wait *months* to get comments on their patches? Not getting any for less than 48h doesn't seem to be saying much. My original problem report was November, 2012: http://www.postgresql.org/message-id/50b3d11f.20...@2ndquadrant.com and my patch to fix this was July 4. Tom gave me a code snipped to test PL/pgSQL's handling of record types. I tested that and it looked ok, but there are other place to test that I don't know about. Why don't you add the proposal to the commitfest? This issue is so much larger than the patch's validity that I don't see how that would work. I obviously can't complete this, so I am adding a TODO item: IS NULL testing of nested ROW() values is inconsistent All my patches are in that thread in case someone who can complete this item wants to take it over. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Sat, Sep 7, 2013 at 10:59:08AM -0400, Bruce Momjian wrote: My original problem report was November, 2012: http://www.postgresql.org/message-id/50b3d11f.20...@2ndquadrant.com and my patch to fix this was July 4. Tom gave me a code snipped to test PL/pgSQL's handling of record types. I tested that and it looked ok, but there are other place to test that I don't know about. Correction --- my updates to Tom's suggestion was only posted September 4. Anyway, it is on the TODO list now. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Thu, Sep 5, 2013 at 05:06:41PM -0400, Bruce Momjian wrote: Another possible fix would be to avoid the IS NULL value optimizer expansion if a ROW construct is inside a ROW(). I have attached a patch that does this for review. Having received no replies, do people perfer this version of the patch that just punts nested ROW IS NULL testing to execQual.c? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us writes: On Thu, Sep 5, 2013 at 05:06:41PM -0400, Bruce Momjian wrote: Another possible fix would be to avoid the IS NULL value optimizer expansion if a ROW construct is inside a ROW(). I have attached a patch that does this for review. Having received no replies, do people perfer this version of the patch that just punts nested ROW IS NULL testing to execQual.c? For some reason I read your previous message as saying you were willing to wait for considered reviews this time. If not, I'll just write a blanket -1 for any version of this patch. I don't think you've shown that this is more spec-compliant than what we had before, nor that you've made all the relevant code (execQual, eval_const_expressions, column NOT NULL constraints, plpgsql variable NOT NULL constraints, maybe other places) mutually consistent. I'm not a fan of incremental improvements in application-visible semantics: if we change this repeatedly over several releases, that's an application author's worst nightmare, because he'll have to try to work with multiple different behaviors. We need to change this *once* and get it right. You haven't proven that this is now right where it wasn't before, and the patch is certainly not so obviously right that it should go in without considered review. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Fri, Sep 6, 2013 at 11:00:24PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Sep 5, 2013 at 05:06:41PM -0400, Bruce Momjian wrote: Another possible fix would be to avoid the IS NULL value optimizer expansion if a ROW construct is inside a ROW(). I have attached a patch that does this for review. Having received no replies, do people perfer this version of the patch that just punts nested ROW IS NULL testing to execQual.c? For some reason I read your previous message as saying you were willing to wait for considered reviews this time. If not, I'll just write a blanket -1 for any version of this patch. Are you saying people will comment later? I wasn't clear that was the plan. I can certainly wait. I don't think you've shown that this is more spec-compliant than what we had before, nor that you've made all the relevant code (execQual, eval_const_expressions, column NOT NULL constraints, plpgsql variable NOT NULL constraints, maybe other places) mutually consistent. I believe all the other places (execQual, plpgsql variables) all treat embedded row in rows as non-null, but I don't even know how to test all the place. Can someone do that? I'm not a fan of incremental improvements in application-visible semantics: if we change this repeatedly over several releases, that's an application author's worst nightmare, because he'll have to try to work with multiple different behaviors. We need to change this *once* and get it right. You haven't proven that this is now right where it wasn't before, and the patch is certainly not so obviously right that it should go in without considered review. Yes, we have to be sure to get this right. However, I am not able to test all the places you have mentioned, so unless someone else finds this important enough to work on, I will just document it as a TODO and close it. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On 2013-09-06 23:07:04 -0400, Bruce Momjian wrote: On Fri, Sep 6, 2013 at 11:00:24PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Sep 5, 2013 at 05:06:41PM -0400, Bruce Momjian wrote: Another possible fix would be to avoid the IS NULL value optimizer expansion if a ROW construct is inside a ROW(). I have attached a patch that does this for review. Having received no replies, do people perfer this version of the patch that just punts nested ROW IS NULL testing to execQual.c? For some reason I read your previous message as saying you were willing to wait for considered reviews this time. If not, I'll just write a blanket -1 for any version of this patch. Are you saying people will comment later? I wasn't clear that was the plan. I can certainly wait. You do realize mere mortals in the project frequently have to wait *months* to get comments on their patches? Not getting any for less than 48h doesn't seem to be saying much. Why don't you add the proposal to the commitfest? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Wed, Sep 4, 2013 at 9:26 PM, Bruce Momjian br...@momjian.us wrote: On Tue, Sep 3, 2013 at 09:32:44PM -0400, Bruce Momjian wrote: In this test, SELECT NULL (which internally would produce SELECT ROW(NULL)), returns TRUE, while SELECT ROW(NULL) and further nesting returns false. This has made me adjust my goal and change it so SELECT ROW(NULL) IS NULL returns true, and any further nesting returns false. Attached is a patch which accomplishes this, and a documentation update. I have not heard any feedback on this patch, so I would like to apply it to give us a nested ROW/IS NULL API we can document. It would have to be marked in the release notes as a backward incompatibility. I don't have time to look at this in detail right now, but I think that's considerably premature. I'm not convinced that we understand all of the problems in this area are yet, let alone the solutions. And I notice that you haven't substantively responded to some of Tom's concerns. So -1 from me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Robert Haas robertmh...@gmail.com writes: On Wed, Sep 4, 2013 at 9:26 PM, Bruce Momjian br...@momjian.us wrote: I have not heard any feedback on this patch, so I would like to apply it to give us a nested ROW/IS NULL API we can document. It would have to be marked in the release notes as a backward incompatibility. I don't have time to look at this in detail right now, but I think that's considerably premature. I'm not convinced that we understand all of the problems in this area are yet, let alone the solutions. And I notice that you haven't substantively responded to some of Tom's concerns. In particular, I don't think it's a good idea to change eval_const_expression's behavior in an incompatible way that simply makes it differently inconsistent with other IS NULL code paths. We should leave things alone until we have a full fix, otherwise we'll just be breaking people's apps repeatedly. I would also say that allowing eval_const_expression to drive what we think the right behavior is is completely backwards, because it's about the least performance-critical case. You should be looking at execQual.c first. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Thu, Sep 5, 2013 at 02:14:39PM -0400, Tom Lane wrote:
Robert Haas robertmh...@gmail.com writes:
On Wed, Sep 4, 2013 at 9:26 PM, Bruce Momjian br...@momjian.us wrote:
I have not heard any feedback on this patch, so I would like to apply it
to give us a nested ROW/IS NULL API we can document. It would have to
be marked in the release notes as a backward incompatibility.
I don't have time to look at this in detail right now, but I think
that's considerably premature. I'm not convinced that we understand
all of the problems in this area are yet, let alone the solutions.
And I notice that you haven't substantively responded to some of Tom's
concerns.
In particular, I don't think it's a good idea to change
eval_const_expression's behavior in an incompatible way that simply
makes it differently inconsistent with other IS NULL code paths.
Let me summarize what eval_const_expressions_mutator() does: it takes a
ROW() IS NULL construct, and converts it to individual _value_ IS NULL
clauses so they can be better optimized. It changes:
ROW(x, ROW(y)) IS NULL
to
x IS NULL AND ROW(y) IS NULL
By removing this one level of ROW construct, it causes ROW(ROW(NULL)) IS
NULL to return true, while more nested cases do not. It also tries to
short-circuit the IS NULL clause if it finds a literal NULL constant
inside the row.
My patch uses this short-circuit logic to return false for IS NULL if it
finds an embedded ROW construct. This makes the code match the code in
ExecEvalNullTest(), which already treats embedded ROW values as
non-null, e.g. it already treats ROW(ROW(x)) IS NULL as false.
Specifically, any code path that doesn't pass through
eval_const_expressions_mutator() and gets to execQual.c will return
false for this.
We should leave things alone until we have a full fix, otherwise we'll
just be breaking people's apps repeatedly.
Yes, we don't want that.
I would also say that allowing eval_const_expression to drive what we
think the right behavior is is completely backwards, because it's
about the least performance-critical case. You should be looking at
execQual.c first.
I am making eval_const_expression match execQual.c, specifically
ExecEvalNullTest(), which calls heap_attisnull(), and looks at the NULL
indicator, which can't be true for an attribute containing a ROW value.
I am confused why others don't see what I am seeing.
Another possible fix would be to avoid the IS NULL value optimizer
expansion if a ROW construct is inside a ROW(). I have attached a patch
that does this for review.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
new file mode 100644
index 76c032c..54e59dc
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** eval_const_expressions_mutator(Node *nod
*** 3100,3105
--- 3100,3122
return makeBoolConst(false, false);
continue;
}
+
+ /* Do we have a ROW() expression in the row? */
+ if (type_is_rowtype(exprType(relem)))
+ {
+ /*
+ * If this ROW() has an embedded ROW() value,
+ * our optimization of removing a level of ROW()
+ * nesting causes ROW(ROW(NULL)) IS NULL to return
+ * true, so just return the original clause.
+ */
+ newntest = makeNode(NullTest);
+ newntest-arg = (Expr *) arg;
+ newntest-nulltesttype = ntest-nulltesttype;
+ newntest-argisrow = ntest-argisrow;
+ return (Node *) newntest;
+ }
+
newntest = makeNode(NullTest);
newntest-arg = (Expr *) relem;
newntest-nulltesttype = ntest-nulltesttype;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 3, 2013 at 09:32:44PM -0400, Bruce Momjian wrote: In this test, SELECT NULL (which internally would produce SELECT ROW(NULL)), returns TRUE, while SELECT ROW(NULL) and further nesting returns false. This has made me adjust my goal and change it so SELECT ROW(NULL) IS NULL returns true, and any further nesting returns false. Attached is a patch which accomplishes this, and a documentation update. I have not heard any feedback on this patch, so I would like to apply it to give us a nested ROW/IS NULL API we can document. It would have to be marked in the release notes as a backward incompatibility. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Fri, Jul 5, 2013 at 10:21:19AM -0400, Bruce Momjian wrote:
On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote:
Bruce Momjian br...@momjian.us writes:
I developed the attached patch which properly recurses into ROW()
records checking for NULLs; you can see it returns the right answer in
all cases (and constant folds too):
My recollection of the previous discussion is that we didn't have
consensus on what the right behavior is, so I'm not sure you can just
assert that this patch is right. In any case this is only touching the
tip of the iceberg. If we intend that rows of nulls should be null,
then we have got issues with, for example, NOT NULL column constraint
checks, which don't have any such recursion built into them. I think
the same is true for plpgsql variable NOT NULL restrictions, and there
are probably some other places.
Well we have three cases:
1 SELECT ROW(NULL) IS NULL;
2 SELECT ROW(ROW(NULL)) IS NULL;
3 SELECT ROW(ROW(ROW(NULL))) IS NULL;
I think we could have them all return false, or all true, or the first
one true, and the rest false. What I don't think we can justify is 1
and 2 as true, and 3 false.
I have done some more research in this, and was able to verify Tom's
concern that PL/pgSQL's IS NULL doesn't recurse into ROW expressions:
DO LANGUAGE plpgsql $$
DECLARE
r RECORD;
BEGIN
SELECT NULL INTO r;
IF (r IS NULL)
THEN RAISE NOTICE 'true';
ELSE RAISE NOTICE 'false';
END IF;
END;
$$;
NOTICE: true
DO
In this test, SELECT NULL (which internally would produce SELECT
ROW(NULL)), returns TRUE, while SELECT ROW(NULL) and further nesting
returns false.
This has made me adjust my goal and change it so SELECT ROW(NULL) IS
NULL returns true, and any further nesting returns false.
Attached is a patch which accomplishes this, and a documentation update.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 00f8ffb..51bfe31
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 351,357
for both tests.
This definition conforms to the SQL standard, and is a change from the
inconsistent behavior exhibited by productnamePostgreSQL/productname
! versions prior to 8.2.
/para
/note
--- 351,359
for both tests.
This definition conforms to the SQL standard, and is a change from the
inconsistent behavior exhibited by productnamePostgreSQL/productname
! versions prior to 8.2. Row expressions inside row
! expressions are processed as non-null, even if the sub-row contains only
! nulls.
/para
/note
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
new file mode 100644
index 76c032c..ce7047f
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** eval_const_expressions_mutator(Node *nod
*** 3100,3109
return makeBoolConst(false, false);
continue;
}
newntest = makeNode(NullTest);
newntest-arg = (Expr *) relem;
newntest-nulltesttype = ntest-nulltesttype;
! newntest-argisrow = type_is_rowtype(exprType(relem));
newargs = lappend(newargs, newntest);
}
/* If all the inputs were constants, result is TRUE */
--- 3100,3124
return makeBoolConst(false, false);
continue;
}
+ /* Do we have a ROW() expression in the row? */
+ if (type_is_rowtype(exprType(relem)))
+ {
+ /*
+ * ROW values in ROW values are not null, even if
+ * those sub-ROW values contain only nulls. This
+ * makes the code match PL/pgSQL and constraint
+ * IS NULL checks which don't probe into sub-ROWs
+ * for NULL values.
+ */
+ if (ntest-nulltesttype == IS_NULL)
+ return makeBoolConst(false, false);
+ continue;
+ }
+
newntest = makeNode(NullTest);
newntest-arg = (Expr *) relem;
newntest-nulltesttype = ntest-nulltesttype;
! newntest-argisrow = false;
newargs = lappend(newargs, newntest);
}
/* If all the inputs were constants, result is TRUE */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us writes: This has made me adjust my goal and change it so SELECT ROW(NULL) IS NULL returns true, and any further nesting returns false. AFAICS, the only good argument for breaking backwards compatibility here is if you can convince people that the new behavior is more conformant to the SQL spec. Where's the chapter and verse that argues for this interpretation? And I will say once more that a patch that affects only the behavior of eval_const_expressions can be rejected on its face. That code has to be kept in sync with the behavior of execQual.c, not just whacked around by itself. And then there are the NOT NULL constraint cases to worry about. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
I wrote: And I will say once more that a patch that affects only the behavior of eval_const_expressions can be rejected on its face. That code has to be kept in sync with the behavior of execQual.c, not just whacked around by itself. And then there are the NOT NULL constraint cases to worry about. Hmm ... actually, it's already not in sync, because: regression=# create table tt (x int); CREATE TABLE regression=# insert into tt values(null); INSERT 0 1 regression=# select row(x) from tt; row - () (1 row) regression=# select row(row(x)) from tt; row (()) (1 row) regression=# select row(row(row(x))) from tt; row -- ((())) (1 row) There's certainly no excuse for this behaving differently from the cases with a simple constant NULL. So I'm a bit inclined to say that we should rip out the special case in eval_const_expressions, not make it even less self-consistent. It's possible to argue that existing applications won't be too sensitive to the behavior of the constant cases, but they surely must be depending on the behavior in the non-constant cases. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 3, 2013 at 8:32 PM, Bruce Momjian br...@momjian.us wrote: On Fri, Jul 5, 2013 at 10:21:19AM -0400, Bruce Momjian wrote: On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: I developed the attached patch which properly recurses into ROW() records checking for NULLs; you can see it returns the right answer in all cases (and constant folds too): My recollection of the previous discussion is that we didn't have consensus on what the right behavior is, so I'm not sure you can just assert that this patch is right. In any case this is only touching the tip of the iceberg. If we intend that rows of nulls should be null, then we have got issues with, for example, NOT NULL column constraint checks, which don't have any such recursion built into them. I think the same is true for plpgsql variable NOT NULL restrictions, and there are probably some other places. Well we have three cases: 1 SELECT ROW(NULL) IS NULL; 2 SELECT ROW(ROW(NULL)) IS NULL; 3 SELECT ROW(ROW(ROW(NULL))) IS NULL; I think we could have them all return false, or all true, or the first one true, and the rest false. What I don't think we can justify is 1 and 2 as true, and 3 false. I have done some more research in this, and was able to verify Tom's concern that PL/pgSQL's IS NULL doesn't recurse into ROW expressions: DO LANGUAGE plpgsql $$ DECLARE r RECORD; BEGIN SELECT NULL INTO r; IF (r IS NULL) THEN RAISE NOTICE 'true'; ELSE RAISE NOTICE 'false'; END IF; END; $$; NOTICE: true DO In this test, SELECT NULL (which internally would produce SELECT ROW(NULL)), returns TRUE, while SELECT ROW(NULL) and further nesting returns false. This has made me adjust my goal and change it so SELECT ROW(NULL) IS NULL returns true, and any further nesting returns false. It gets worse and worse. The IS NULL operator is already pretty much special cased -- in just about all other case concerning rowtypes (for example coalesce) 'null containing rowtypes are *not* considered to be null as the container itself has a null bit independent of it's elements which is expressly contrary to the SQL standard. This is tragic; postgres's way of doing it (except with IS NULL) makes an awful lot of sense to me but here we are. I think before making any behavior changes at all, we need to ask ourselves: 1. Are we willing to break compatibility in order to move to spec compliant behavior? 2. and if so, what mountains do we have to cross to get there? Your proposed change (implementation details aside) seems ok in the sense that it doesn't seem to have a lot of obvious side effects but the elephant in the room is #1; if the answer is 'no', then I'd say the best course of action is to let things be. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 3, 2013 at 09:43:14PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: This has made me adjust my goal and change it so SELECT ROW(NULL) IS NULL returns true, and any further nesting returns false. AFAICS, the only good argument for breaking backwards compatibility here is if you can convince people that the new behavior is more conformant to the SQL spec. Where's the chapter and verse that argues for this interpretation? The SQL03 standard in section 8.7, table 14, says degree 1: null and degree 1: all null. Does that mean they are considering nested rows as degree 1, or is that the number of values in the row? A footnote says: For all R, R IS NOT NULL has the same result as NOT R IS NULL if and only if R is of degree 1. which seems to support the idea that degree is the number of values, meaning they don't discuss nesting. And I will say once more that a patch that affects only the behavior of eval_const_expressions can be rejected on its face. That code has to be kept in sync with the behavior of execQual.c, not just whacked around by itself. And then there are the NOT NULL constraint cases to worry about. I thought execQual.c was already not recursing so I didn't see a need to change that. I could not figure out how to test a NOT NULL constraint for nesting. What is driving my research here is that our current behavior is really not documentable. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 3, 2013 at 10:27:38PM -0400, Tom Lane wrote: I wrote: And I will say once more that a patch that affects only the behavior of eval_const_expressions can be rejected on its face. That code has to be kept in sync with the behavior of execQual.c, not just whacked around by itself. And then there are the NOT NULL constraint cases to worry about. Hmm ... actually, it's already not in sync, because: regression=# create table tt (x int); CREATE TABLE regression=# insert into tt values(null); INSERT 0 1 regression=# select row(x) from tt; row - () (1 row) regression=# select row(row(x)) from tt; row (()) (1 row) regression=# select row(row(row(x))) from tt; row -- ((())) (1 row) Uh, I see the same output you show for a NULL constant: SELECT ROW(NULL); row - () SELECT ROW(ROW(NULL)); row (()) SELECT ROW(ROW(ROW(NULL))); row -- ((())) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Tue, Sep 3, 2013 at 09:44:33PM -0500, Merlin Moncure wrote: It gets worse and worse. The IS NULL operator is already pretty much special cased -- in just about all other case concerning rowtypes (for example coalesce) 'null containing rowtypes are *not* considered to be null as the container itself has a null bit independent of it's elements which is expressly contrary to the SQL standard. This is tragic; postgres's way of doing it (except with IS NULL) makes an awful lot of sense to me but here we are. I think before making any behavior changes at all, we need to ask ourselves: 1. Are we willing to break compatibility in order to move to spec compliant behavior? 2. and if so, what mountains do we have to cross to get there? Your proposed change (implementation details aside) seems ok in the sense that it doesn't seem to have a lot of obvious side effects but the elephant in the room is #1; if the answer is 'no', then I'd say the best course of action is to let things be. Yes, these are the right questions. If we leave things unchanged, can we document the behavior we currently have? What I _think_ we have now is that IS NULL in queries checks two levels deep for nulls, but checks only one level deep for PL/pgSQL and constraints. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Peter Eisentraut wrote:
On 7/4/13 5:06 PM, Alvaro Herrera wrote:
FWIW if changing the behavior of NOT NULL constraints is desired, I
still have the patch to catalogue them around, if anyone wants to play
around. I haven't gotten around to finishing it up, yet :-(
If your latest patch isn't publicly available, I'd like to see it. I
might work on that later this year.
Here it is. Note that it is quite incomplete: there are parts that are
not even close to compiling. I think I was trying to figure out how to
determine whether a column was of a composite type.
I might get back to it eventually, so if you start working on it, do let
me know.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
diff --git a/src/backend/commands/constraint.c b/src/backend/commands/constraint.c
index 8ac8373..74f0766 100644
--- a/src/backend/commands/constraint.c
+++ b/src/backend/commands/constraint.c
@@ -14,12 +14,16 @@
#include postgres.h
#include catalog/index.h
+#include catalog/pg_constraint.h
+#include commands/constraint.h
#include commands/trigger.h
#include executor/executor.h
#include utils/builtins.h
#include utils/rel.h
+#include utils/syscache.h
#include utils/tqual.h
+static char *tryExtractNotNull_internal(Node *node, Relation rel);
/*
* unique_key_recheck - trigger function to do a deferred uniqueness check.
@@ -188,3 +192,166 @@ unique_key_recheck(PG_FUNCTION_ARGS)
return PointerGetDatum(NULL);
}
+
+/*
+ * Create a Constraint node representing a col IS NOT NULL expression
+ * using a ColumnRef within, for the given relation and column.
+ *
+ * If the constraint name is not provided, it is generated.
+ */
+Constraint *
+createCheckNotNullConstraint(Oid nspid, char *constraint_name,
+ const char *relname, const char *colname)
+{
+ ColumnRef *colref;
+ NullTest *nulltest;
+
+ colref = (ColumnRef *) makeNode(ColumnRef);
+ colref-fields = list_make1(makeString(pstrdup(colname)));
+
+ nulltest = (NullTest *) makeNode(NullTest);
+ nulltest-argisrow = false; /* FIXME -- may be bogus! */
+ nulltest-nulltesttype = IS_NOT_NULL;
+ nulltest-arg = (Expr *) colref;
+
+ return makeCheckConstraint(nspid, constraint_name, relname, colname,
+ nulltest);
+}
+
+/*
+ * Create a Constraint node representing col IS DISTINCT FROM NULL. This is
+ * just like the above, but this is intended to be used for columns with
+ * composite types, which have slightly different rules.
+ */
+Constraint *
+createCheckDistinctNotNullConstraint(Oid nspid, char *constraint_name,
+ const char *relname, const char *colname,
+ Oid column_type)
+{
+ Constraint *check;
+ ColumnRef *colref;
+ A_Expr *expr;
+
+ colref = (ColumnRef *) makeNode(ColumnRef);
+ colref-fields = list_make1(makeString(pstrdup(colname)));
+
+ expr = makeSimpleA_Expr(AEXPR_DISTINCT, =, colref, makeNullAConst(-1),
+ -1);
+
+ return makeCheckConstraint(nspid, constraint_name, relname, colname, expr);
+
+ return check;
+}
+
+static Constraint *
+makeCheckConstraint(Oid nspid, char *constraint_name, const char *relname,
+ const char *colname, Node *expr)
+{
+ Constraint *check = makeNode(Constraint);
+
+ check-contype = CONSTR_CHECK;
+ check-location = -1;
+ check-conname = constraint_name ? constraint_name :
+ ChooseConstraintName(relname, colname, not_null, nspid,
+ NIL);
+ check-raw_expr = raw_expr;
+ check-cooked_expr = NULL;
+
+}
+
+/*
+ * Given a CHECK constraint, examine it and determine whether it is CHECK (col
+ * IS NOT NULL). If it is, return the column name for which it is. Otherwise
+ * return NULL.
+ */
+char *
+tryExtractNotNullFromCheckConstr(Constraint *constr)
+{
+ char *retval;
+
+ Assert(constr-contype == CONSTR_CHECK);
+
+ if (constr-raw_expr != NULL)
+ {
+ retval = tryExtractNotNull_internal(constr-raw_expr, NULL);
+ if (retval != NULL)
+ return retval;
+ }
+
+ if (constr-cooked_expr != NULL)
+ {
+ return tryExtractNotNull_internal(stringToNode(constr-cooked_expr),
+ NULL);
+ }
+
+ return NULL;
+}
+
+/*
+ * As above, but use a pg_constraint row as input.
+ *
+ * tupdesc is pg_constraint's tuple descriptor, and rel is the relation the
+ * constraint is for.
+ */
+char *
+tryExtractNotNullFromCatalog(HeapTuple constrTup, TupleDesc tupdesc,
+ Relation rel)
+{
+ Datum val;
+ bool isnull;
+ char *conbin;
+ Node *node;
+
+ val = SysCacheGetAttr(CONSTROID, constrTup, Anum_pg_constraint_conbin,
+ isnull);
+ if (isnull)
+ elog(ERROR, null conbin for constraint %u,
+ HeapTupleGetOid(constrTup));
+ conbin = TextDatumGetCString(val);
+ node = (Node *) stringToNode(conbin);
+
+ return tryExtractNotNull_internal(node, rel);
+}
+
+/*
+ * Worker for the above
+ */
+static char *
+tryExtractNotNull_internal(Node *node, Relation rel)
+{
+ if (IsA(node, NullTest))
+ {
+ NullTest *nulltest = (NullTest *) node;
+
+ if (nulltest-nulltesttype == IS_NOT_NULL)
+ {
+ if
Re: [HACKERS] strange IS NULL behaviour
On Fri, Jul 5, 2013 at 11:03:56AM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote: No, it isn't, or at least it's far from the only place. If we're going to change this, we would also want to change the behavior of tests on RECORD values, which is something that would have to happen at runtime. I checked RECORD and that behaves with recursion: Apparently you don't even understand the problem. All of these examples you're showing are constants. Try something like declare r record; ... select ... into r ... if (r is null) ... OK, I created the following test on git head (without my patch), and the results look correct: DO LANGUAGE plpgsql $$ DECLARE r RECORD; BEGIN DROP TABLE IF EXISTS test; CREATE TABLE test (x INT, y INT); INSERT INTO test VALUES (1, NULL), (NULL, 1), (NULL, NULL); FOR r IN SELECT * FROM test LOOP IF (r IS NULL) THEN RAISE NOTICE 'true'; ELSE RAISE NOTICE 'false'; END IF; END LOOP; END; $$; NOTICE: false NOTICE: false NOTICE: true Am I missing something? Is this an example of NOT NULL contraints not testing NULLs? CREATE TABLE test3(x INT, y INT); CREATE TABLE test5(z test3 NOT NULL); INSERT INTO test5 VALUES (ROW(NULL, NULL)); SELECT * FROM test5; z - (,) Looks like I have to modify ExecEvalNullTest(). If I fix this, is it going to cause problems with pg_upgraded databases now having values that are no longer validated by the NOT NULL constraint? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Sun, Jul 7, 2013 at 01:04:05PM -0400, Bruce Momjian wrote: Looks like I have to modify ExecEvalNullTest(). Oops, I mean ExecConstraints(). This could be tricky. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On 7/4/13 5:06 PM, Alvaro Herrera wrote: FWIW if changing the behavior of NOT NULL constraints is desired, I still have the patch to catalogue them around, if anyone wants to play around. I haven't gotten around to finishing it up, yet :-( If your latest patch isn't publicly available, I'd like to see it. I might work on that later this year. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: I developed the attached patch which properly recurses into ROW() records checking for NULLs; you can see it returns the right answer in all cases (and constant folds too): My recollection of the previous discussion is that we didn't have consensus on what the right behavior is, so I'm not sure you can just assert that this patch is right. In any case this is only touching the tip of the iceberg. If we intend that rows of nulls should be null, then we have got issues with, for example, NOT NULL column constraint checks, which don't have any such recursion built into them. I think the same is true for plpgsql variable NOT NULL restrictions, and there are probably some other places. Well we have three cases: 1 SELECT ROW(NULL) IS NULL; 2 SELECT ROW(ROW(NULL)) IS NULL; 3 SELECT ROW(ROW(ROW(NULL))) IS NULL; I think we could have them all return false, or all true, or the first one true, and the rest false. What I don't think we can justify is 1 and 2 as true, and 3 false. Can someone show how those others behave? I don't know enough to test it. The optimizer seems like the right place to fix this, per my patch. No, it isn't, or at least it's far from the only place. If we're going to change this, we would also want to change the behavior of tests on RECORD values, which is something that would have to happen at runtime. I checked RECORD and that behaves with recursion: SELECT RECORD(NULL) IS NULL; ?column? -- t SELECT RECORD(RECORD(NULL)) IS NULL; ?column? -- t SELECT RECORD(RECORD(RECORD(NULL))) IS NULL; ?column? -- t Am I missing something? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us writes: On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote: No, it isn't, or at least it's far from the only place. If we're going to change this, we would also want to change the behavior of tests on RECORD values, which is something that would have to happen at runtime. I checked RECORD and that behaves with recursion: Apparently you don't even understand the problem. All of these examples you're showing are constants. Try something like declare r record; ... select ... into r ... if (r is null) ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Fri, Jul 5, 2013 at 11:03:56AM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Jul 4, 2013 at 04:29:20PM -0400, Tom Lane wrote: No, it isn't, or at least it's far from the only place. If we're going to change this, we would also want to change the behavior of tests on RECORD values, which is something that would have to happen at runtime. I checked RECORD and that behaves with recursion: Apparently you don't even understand the problem. All of these examples you're showing are constants. Try something like declare r record; ... select ... into r ... if (r is null) ... Not aparently --- I already said I didn't understand the problem. Should I just mark this as a TODO? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us writes: Should I just mark this as a TODO? I thought it was on the list already. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Fri, Jul 5, 2013 at 12:43:57PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: Should I just mark this as a TODO? I thought it was on the list already. We only have: Improve handling of NULLs in arrays and some pl/pgsql items regarding nulls. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
On Mon, Nov 26, 2012 at 09:29:19PM +0100, Hannu Krosing wrote: On 11/26/2012 09:05 PM, Tom Lane wrote: Hannu Krosing ha...@2ndquadrant.com writes: In some previous mail Tom Lane claimed that by SQL standard either an array of all NULLs or a record with all fields NULLs (I don't remember which) is also considered NULL. If this is true, then an empty array - which can be said to consist of nothing but NULLs - should itself be NULL. What I think you're referring to is that the spec says that foo IS NULL should return true if foo is a record containing only null fields. Is this requirement recursive ? That is , should ROW(NULL, NULL, ROW(NULL, ROW(NULL, NULL))) IS NULL also be true ? Currently PostgreSQL does this kind of IS NULL for simple rows hannu=# SELECT ROW(NULL, NULL) IS NULL; ?column? -- t (1 row) and also for first level row types hannu=# SELECT ROW(NULL, ROW(NULL, NULL)) IS NULL; ?column? -- t (1 row) but then mysteriously stops working at third level hannu=# SELECT ROW(NULL, NULL, ROW(NULL, ROW(NULL, NULL))) IS NULL; ?column? -- f (1 row) I finally had time to look at this, and it is surprising. I used EXPLAIN VERBOSE to see what the optimizer was outputting: EXPLAIN VERBOSE SELECT ROW(null) IS NULL; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) --Output: true EXPLAIN VERBOSE SELECT ROW(ROW(null)) IS NULL; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) --Output: (ROW(NULL::unknown) IS NULL) EXPLAIN VERBOSE SELECT ROW(ROW(ROW(null))) IS NULL; QUERY PLAN - Result (cost=0.00..0.01 rows=1 width=0) --Output: (ROW(ROW(NULL::unknown)) IS NULL) The first test outputs a constant, 'true'. The second test is ROW(NULL::unknown) because the inner ROW(NULL) was converted to NULL:unknown. The third one, which returns false (wrong), happens because you have ROW embedded in ROW, which the optimizer can't process, and the executor can't either. I developed the attached patch which properly recurses into ROW() records checking for NULLs; you can see it returns the right answer in all cases (and constant folds too): test= EXPLAIN VERBOSE SELECT ROW(null) IS NULL; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) --Output: true (2 rows) test= EXPLAIN VERBOSE SELECT ROW(ROW(null)) IS NULL; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) --Output: true (2 rows) test= EXPLAIN VERBOSE SELECT ROW(ROW(ROW(null))) IS NULL; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) --Output: true (2 rows) You might think the problem is only with constants, but it extends to column values too (non-patched server): CREATE TABLE test (x INT); CREATE TABLE INSERT INTO test VALUES (1), (NULL); INSERT 0 2 SELECT ROW(x) IS NULL FROM test; ?column? -- f t SELECT ROW(ROW(x)) IS NULL FROM test; ?column? -- f t SELECT ROW(ROW(ROW(x))) IS NULL FROM test; ?column? -- -- f -- f With the patch, that works too: SELECT ROW(ROW(ROW(x))) IS NULL FROM test; ?column? -- f t The optimizer seems like the right place to fix this, per my patch. It already flattens IS NULL tests into a series of AND clauses, and now by recursing, it handles nested ROW values properly too. This fix would be for head only. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c new file mode 100644 index 6d5b204..0abf789 *** a/src/backend/optimizer/util/clauses.c --- b/src/backend/optimizer/util/clauses.c *** eval_const_expressions_mutator(Node *nod *** 3149,3155 newntest-arg = (Expr *) relem; newntest-nulltesttype = ntest-nulltesttype; newntest-argisrow = type_is_rowtype(exprType(relem)); ! newargs = lappend(newargs, newntest); } /* If all the inputs were constants, result is TRUE */ if (newargs == NIL) --- 3149,3160 newntest-arg = (Expr *) relem; newntest-nulltesttype =
Re: [HACKERS] strange IS NULL behaviour
Bruce Momjian br...@momjian.us writes: I developed the attached patch which properly recurses into ROW() records checking for NULLs; you can see it returns the right answer in all cases (and constant folds too): My recollection of the previous discussion is that we didn't have consensus on what the right behavior is, so I'm not sure you can just assert that this patch is right. In any case this is only touching the tip of the iceberg. If we intend that rows of nulls should be null, then we have got issues with, for example, NOT NULL column constraint checks, which don't have any such recursion built into them. I think the same is true for plpgsql variable NOT NULL restrictions, and there are probably some other places. The optimizer seems like the right place to fix this, per my patch. No, it isn't, or at least it's far from the only place. If we're going to change this, we would also want to change the behavior of tests on RECORD values, which is something that would have to happen at runtime. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] strange IS NULL behaviour
Tom Lane wrote: My recollection of the previous discussion is that we didn't have consensus on what the right behavior is, so I'm not sure you can just assert that this patch is right. In any case this is only touching the tip of the iceberg. If we intend that rows of nulls should be null, then we have got issues with, for example, NOT NULL column constraint checks, which don't have any such recursion built into them. FWIW if changing the behavior of NOT NULL constraints is desired, I still have the patch to catalogue them around, if anyone wants to play around. I haven't gotten around to finishing it up, yet :-( -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
