Thread: Re: [BUGS] BUG #13148: Unexpected deferred EXCLUDE constraint violation on derived table

postgresql2@realityexists.net writes:
> I have a deferred EXCLUDE constraint on a derived table. Inside a
> transaction I insert a new row that conflicts with an existing one (so the
> constraint would fail if it was immediate), delete the old row and run an
> unrelated UPDATE on the new row, then try to commit. I would expect the
> commit to succeed, since there is now no conflict, but it fails with
>     ERROR: conflicting key value violates exclusion constraint
> "uq_derived_timeslice_dup_time_ex"

Hm.  The given test case is overcomplicated; in point of fact it will fail
on any deferred exclusion constraint, eg


DROP TABLE IF EXISTS derived_timeslice CASCADE;

CREATE TABLE derived_timeslice
(
  timeslice_id integer NOT NULL,
  feature_id integer NOT NULL,
  name text,
  CONSTRAINT pk_derived_timeslice PRIMARY KEY (timeslice_id),
  CONSTRAINT uq_derived_timeslice_dup_time_ex EXCLUDE
    USING btree (feature_id WITH =)
    DEFERRABLE INITIALLY DEFERRED
);

INSERT INTO derived_timeslice (timeslice_id, feature_id) VALUES (51, 1);

BEGIN;

-- Insert row that violates deferred constraint
INSERT INTO derived_timeslice (timeslice_id, feature_id) VALUES (52, 1);

-- Delete the old row - now there should be no more conflict
DELETE FROM derived_timeslice WHERE timeslice_id = 51;

-- Problem doesn't occur without an UPDATE statement
UPDATE derived_timeslice SET name = 'Updated' WHERE timeslice_id = 52;

-- This confirms there is only 1 row - no conflict
SELECT * FROM derived_timeslice;

COMMIT; -- Enforce constraint - error occurs here


The cause of the problem seems to be that the UPDATE performs a HOT update
of the new tuple, leaving in this case a dead tuple at (0,2) that is HOT
updated by (0,3).  When unique_key_recheck() is invoked for (0,2), it
believes, correctly, that it has to perform the recheck anyway ... but it
tells check_exclusion_constraint that the check is being performed for
(0,2).  So the index search inside check_exclusion_constraint finds the
live tuple at (0,3) and thinks that is a conflict.

This is reproducible clear back to 9.0 where exclusion constraints were
added.

The easiest fix seems to be to pass the HOT child's TID instead of the
TID we were called for.  (Note that the other path, for a regular unique
constraint, is correct as-is because the original TID is what the index
will know about.)

The attached patch seems to fix the problem without breaking any existing
regression tests, but I wonder if anyone can see a hole in it.

            regards, tom lane

diff --git a/src/backend/commands/constraint.c b/src/backend/commands/constraint.c
index e49affb..28fccaf 100644
*** a/src/backend/commands/constraint.c
--- b/src/backend/commands/constraint.c
*************** unique_key_recheck(PG_FUNCTION_ARGS)
*** 89,97 ****
       * because this trigger gets queued only in response to index insertions;
       * which means it does not get queued for HOT updates.  The row we are
       * called for might now be dead, but have a live HOT child, in which case
!      * we still need to make the check.  Therefore we have to use
!      * heap_hot_search, not just HeapTupleSatisfiesVisibility as is done in
!      * the comparable test in RI_FKey_check.
       *
       * This might look like just an optimization, because the index AM will
       * make this identical test before throwing an error.  But it's actually
--- 89,98 ----
       * because this trigger gets queued only in response to index insertions;
       * which means it does not get queued for HOT updates.  The row we are
       * called for might now be dead, but have a live HOT child, in which case
!      * we still need to make the check --- effectively, we're applying the
!      * check against the live child row, although we can use the values from
!      * this row since by definition all columns of interest to us are the
!      * same.
       *
       * This might look like just an optimization, because the index AM will
       * make this identical test before throwing an error.  But it's actually
*************** unique_key_recheck(PG_FUNCTION_ARGS)
*** 159,165 ****
      {
          /*
           * Note: this is not a real insert; it is a check that the index entry
!          * that has already been inserted is unique.
           */
          index_insert(indexRel, values, isnull, &(new_row->t_self),
                       trigdata->tg_relation, UNIQUE_CHECK_EXISTING);
--- 160,168 ----
      {
          /*
           * Note: this is not a real insert; it is a check that the index entry
!          * that has already been inserted is unique.  Passing t_self is
!          * correct even if t_self is now dead, because that is the TID the
!          * index will know about.
           */
          index_insert(indexRel, values, isnull, &(new_row->t_self),
                       trigdata->tg_relation, UNIQUE_CHECK_EXISTING);
*************** unique_key_recheck(PG_FUNCTION_ARGS)
*** 168,177 ****
      {
          /*
           * For exclusion constraints we just do the normal check, but now it's
!          * okay to throw error.
           */
          check_exclusion_constraint(trigdata->tg_relation, indexRel, indexInfo,
!                                    &(new_row->t_self), values, isnull,
                                     estate, false);
      }

--- 171,182 ----
      {
          /*
           * For exclusion constraints we just do the normal check, but now it's
!          * okay to throw error.  In the HOT-update case, we must use the live
!          * HOT child's TID here, else check_exclusion_constraint will think
!          * the child is a conflict.
           */
          check_exclusion_constraint(trigdata->tg_relation, indexRel, indexInfo,
!                                    &tmptid, values, isnull,
                                     estate, false);
      }

Hi,

On 2015-05-10 16:01:53 -0400, Tom Lane wrote:
> The cause of the problem seems to be that the UPDATE performs a HOT update
> of the new tuple, leaving in this case a dead tuple at (0,2) that is HOT
> updated by (0,3).  When unique_key_recheck() is invoked for (0,2), it
> believes, correctly, that it has to perform the recheck anyway ... but it
> tells check_exclusion_constraint that the check is being performed for
> (0,2).  So the index search inside check_exclusion_constraint finds the
> live tuple at (0,3) and thinks that is a conflict.

Heh, it's curious that this wasn't found up until now. I also wonder if
this might be related to the spurious violations Peter G. has been
observing...

> The attached patch seems to fix the problem without breaking any existing
> regression tests, but I wonder if anyone can see a hole in it.

Looks good to me.

Greetings,

Andres Freund

On Mon, May 11, 2015 at 9:47 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-05-10 16:01:53 -0400, Tom Lane wrote:
>> The cause of the problem seems to be that the UPDATE performs a HOT update
>> of the new tuple, leaving in this case a dead tuple at (0,2) that is HOT
>> updated by (0,3).  When unique_key_recheck() is invoked for (0,2), it
>> believes, correctly, that it has to perform the recheck anyway ... but it
>> tells check_exclusion_constraint that the check is being performed for
>> (0,2).  So the index search inside check_exclusion_constraint finds the
>> live tuple at (0,3) and thinks that is a conflict.
>
> Heh, it's curious that this wasn't found up until now. I also wonder if
> this might be related to the spurious violations Peter G. has been
> observing...

I don't think so. Speculative insertion relies on the assumption that
the speculatively inserted tuple isn't MVCC visible to other sessions.
I actually prototyped an implementation that avoided the historic
"unprincipled deadlocks" of exclusion constraints (a known limitation
since they were added), by making *UPDATE* also do a speculative
insertion, and by making even non-UPSERT INSERTs insert speculatively.

This almost worked, but when time came to back out of a speculative
insertion on an UPDATE due to a conflict from a concurrent session,
the implementation couldn't handle it - it was just a mess to try and
figure out how that was supposed to work with heap_update(), and so
that prototype was scrapped.

For the benefit of those not closely involved in the ON CONFLICT
project, I should point out that ON CONFLICT will not accept a
deferred index as an arbiter index.
-- 
Peter Geoghegan