BUG #17558: 15beta2: Endless loop with UNIQUE NULLS NOT DISTINCT and INSERT ... ON CONFLICT

PG Bug reporting form <noreply@postgresql.org> writes:

In PostgreSQL 15 beta, when I try to insert a NULL value which violates a
"UNIQUE NULLS NOT DISTINCT" constraint, PostgreSQL will get stuck in an
endless loop if the command includes an "ON CONFLICT" clause.

CREATE TABLE test (val TEXT UNIQUE NULLS NOT DISTINCT);
INSERT INTO test (val) VALUES ('a') ON CONFLICT DO NOTHING; -- inserts 'a'
INSERT INTO test (val) VALUES ('a') ON CONFLICT DO NOTHING; -- does
nothing
INSERT INTO test (val) VALUES (NULL) ON CONFLICT DO NOTHING; -- inserts
NULL
INSERT INTO test (val) VALUES (NULL) ON CONFLICT DO NOTHING; --
unresponsive

Yup, still a problem in HEAD. It correctly reports an error if
you just "INSERT INTO test (val) VALUES (NULL)", but something
in the ON CONFLICT code path seems not to be on board with this.
Peter?

regards, tom lane

On Mon, Jul 25, 2022 at 11:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

In PostgreSQL 15 beta, when I try to insert a NULL value which violates a
"UNIQUE NULLS NOT DISTINCT" constraint, PostgreSQL will get stuck in an
endless loop if the command includes an "ON CONFLICT" clause.

CREATE TABLE test (val TEXT UNIQUE NULLS NOT DISTINCT);
INSERT INTO test (val) VALUES ('a') ON CONFLICT DO NOTHING; -- inserts

'a'

INSERT INTO test (val) VALUES ('a') ON CONFLICT DO NOTHING; -- does
nothing
INSERT INTO test (val) VALUES (NULL) ON CONFLICT DO NOTHING; -- inserts
NULL
INSERT INTO test (val) VALUES (NULL) ON CONFLICT DO NOTHING; --
unresponsive

Yup, still a problem in HEAD. It correctly reports an error if
you just "INSERT INTO test (val) VALUES (NULL)", but something
in the ON CONFLICT code path seems not to be on board with this.
Peter?

Yeah, I can see the same problem. _bt_check_unique() can catch violation
of unique index constraint successfully, so insert without 'ON CONFLICT'
has no problem. But ExecCheckIndexConstraints() fails to tell the NULL
tuple violates unique constraint, and the logic below is quite
suspicious to me.

/*
* If any of the input values are NULL, the constraint check is assumed to
* pass (i.e., we assume the operators are strict).
*/
for (i = 0; i < indnkeyatts; i++)
{
if (isnull[i])
return true;
}

Thanks
Richard

On Tue, Jul 26, 2022 at 11:17:45AM +0800, Richard Guo wrote:

Yeah, I can see the same problem. _bt_check_unique() can catch violation
of unique index constraint successfully, so insert without 'ON CONFLICT'
has no problem. But ExecCheckIndexConstraints() fails to tell the NULL
tuple violates unique constraint, and the logic below is quite
suspicious to me.

This is still an open item assigned to Peter, and beta3 is planned for
the 11th of August. Would a resolution by the beginning of next week
be doable?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Tue, Jul 26, 2022 at 11:17:45AM +0800, Richard Guo wrote:

Yeah, I can see the same problem. _bt_check_unique() can catch violation
of unique index constraint successfully, so insert without 'ON CONFLICT'
has no problem. But ExecCheckIndexConstraints() fails to tell the NULL
tuple violates unique constraint, and the logic below is quite
suspicious to me.

This is still an open item assigned to Peter, and beta3 is planned for
the 11th of August. Would a resolution by the beginning of next week
be doable?

Since nothing seems to be happening here, I looked into it. I think
Richard has correctly fingered the buggy code: we need to teach
check_exclusion_or_unique_constraint() that a NULL isn't necessarily
an instant pass. As attached.

Since the presented test case caused an uninterruptible loop, I also added
a CHECK_FOR_INTERRUPTS() that could stop it. (I was initially pretty
surprised that we could get through an index scan without hitting any CFI,
but on reflection it's less surprising, because no index scan actually
happens: check_exclusion_or_unique_constraint() short-circuits things
before very much code has been reached.) I'm kind of inclined to
back-patch the nodeModifyTable.c change all the way, just for safety.

regards, tom lane