Обсуждение: Unique constraint error instead of serialization_failure

Tested PostgreSQL versions: 10.7, 12.2

Hello, I have found an example where I expect to get a serialization failure
error, but instead receive a unique constraint error. My understanding is that
this is not supposed to happen starting with PostgreSQL version 9.6 because of
this patch:

<https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcff8a575198478023ada8a48e13b50f70054766>

The example has a table where each "item_id" has a single version, and we want
to increment the version of one of the items.

Here is the full session that demonstrates the error. Notice at the very end
we get a "unique constraint" error. If we run the example from the beginning,
this time without the "UNIQUE (item_id, version)" constraint, then at the end
we get a serialization failure as expected.

--
-- Setup:
--
CREATE TABLE t (
    item_id INT NOT NULL,
    version INT NOT NULL,
    created_at TIMESTAMPTZ NOT NULL,
    UNIQUE (item_id, version),
    UNIQUE (item_id, created_at)
);
INSERT INTO t (item_id, version, created_at) VALUES
    (10, 1, now() - INTERVAL '2 SECOND'),
    (10, 2, now() - INTERVAL '1 SECOND');

--                                |
-- Transaction 1 --               |  -- Transaction 2 --
--                                |
START TRANSACTION
    ISOLATION LEVEL SERIALIZABLE;

                                    START TRANSACTION
                                        ISOLATION LEVEL SERIALIZABLE;

SELECT version FROM t
WHERE NOT EXISTS(
        SELECT 1 FROM t t2
        WHERE t.item_id = t2.item_id
        AND t.created_at < t2.created_at)
    AND item_id = 10;
-- Result: "2"

                                    SELECT version FROM t
                                    WHERE NOT EXISTS(
                                            SELECT 1 FROM t t2
                                            WHERE t.item_id = t2.item_id
                                            AND t.created_at < t2.created_at)
                                        AND item_id = 10;
                                    -- Result: "2"

-- Insert next value: 2 + 1:
INSERT INTO t
    (item_id, version, created_at)
    VALUES (10, 3, now());

COMMIT;

                                    -- Insert next value: 2 + 1:
                                    INSERT INTO t
                                        (item_id, version, created_at)
                                        VALUES (10, 3, now());
                                    -- ERROR:  duplicate key value violates
                                    --   unique constraint
                                    --   "t_item_id_version_key"
                                    -- DETAIL:  Key
                                    --   (item_id, version)=(10, 3)
                                    --   already exists.

On Thu, May 21, 2020 at 9:24 AM Benny Kramek <benny@medflyt.com> wrote:
> Tested PostgreSQL versions: 10.7, 12.2
>
> Hello, I have found an example where I expect to get a serialization failure
> error, but instead receive a unique constraint error. My understanding is that
> this is not supposed to happen starting with PostgreSQL version 9.6 because of
> this patch:
>
> <https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcff8a575198478023ada8a48e13b50f70054766>

Agreed.  Thanks for the test case.  I hope to look into this in a few
days, but as a kind of Post-It note to remind me, here is your test
case in the form of an isolation test.  Clearly it hasn't quite formed
the dangerous structure at the point the UCV is raised.  Perhaps where
that commit added the extra CheckForSerializableConflictIn() call, it
also needs to ask the table AM to do the same sort of thing for its
tuple?  Not sure without doing some digging.  That'd be similar to
(but not the same as) the AM-assisted SSI checks I contemplated to
make this work for exclude constraints too.

https://www.postgresql.org/message-id/flat/CAMTXbE-sq9JoihvG-ccC70jpjMr%2BDWmnYUj%2BVdnFRFSRuaaLZQ%40mail.gmail.com

On Thu, May 21, 2020 at 1:46 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, May 21, 2020 at 9:24 AM Benny Kramek <benny@medflyt.com> wrote:
> > Tested PostgreSQL versions: 10.7, 12.2
> >
> > Hello, I have found an example where I expect to get a serialization failure
> > error, but instead receive a unique constraint error. My understanding is that
> > this is not supposed to happen starting with PostgreSQL version 9.6 because of
> > this patch:
> >
> > <https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcff8a575198478023ada8a48e13b50f70054766>
>
> Agreed.  Thanks for the test case.  I hope to look into this in a few
> days, but as a kind of Post-It note to remind me, here is your test
> case in the form of an isolation test.  Clearly it hasn't quite formed
> the dangerous structure at the point the UCV is raised.  Perhaps where
> that commit added the extra CheckForSerializableConflictIn() call, it
> also needs to ask the table AM to do the same sort of thing for its
> tuple?  Not sure without doing some digging.  That'd be similar to
> (but not the same as) the AM-assisted SSI checks I contemplated to
> make this work for exclude constraints too.

Ok I think I see what's happening here.  We p-lock a page in
t_item_id_created_at_key, but not t_item_id_version_key.  Then the UCV
happens on t_item_id_version_key.  If you reverse the order of the
UNIQUE clauses in the CREATE statement, you get a serialization
failure instead.  Perhaps to fix this we'd need a way for
_bt_insert(), having decided that it's going to raise a UCV, to run
around and tell ALL relevant indexes to perform a
CheckForSerializableConflictIn() on the pages they would touch, so
that the behaviour doesn't depend on the order we insert into indexes.
Perhaps that would fit well with the plan I mentioned on that other
thread about GIST exclusion constraints, to introduce an
"amwouldinsert()" ("amwouldhaveinserted()"?!) entry point.  It'd give
AMs a chance to raise SSI errors, in error paths, to give error
reporting priority to SSI.

On Fri, May 22, 2020 at 11:45 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, May 21, 2020 at 1:46 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > On Thu, May 21, 2020 at 9:24 AM Benny Kramek <benny@medflyt.com> wrote:
> > > Tested PostgreSQL versions: 10.7, 12.2
> > >
> > > Hello, I have found an example where I expect to get a serialization failure
> > > error, but instead receive a unique constraint error. My understanding is that
> > > this is not supposed to happen starting with PostgreSQL version 9.6 because of
> > > this patch:
> > >
> > > <https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcff8a575198478023ada8a48e13b50f70054766>
> >
> > Agreed.  Thanks for the test case.  I hope to look into this in a few
> > days, but as a kind of Post-It note to remind me, here is your test
> > case in the form of an isolation test.  Clearly it hasn't quite formed
> > the dangerous structure at the point the UCV is raised.  Perhaps where
> > that commit added the extra CheckForSerializableConflictIn() call, it
> > also needs to ask the table AM to do the same sort of thing for its
> > tuple?  Not sure without doing some digging.  That'd be similar to
> > (but not the same as) the AM-assisted SSI checks I contemplated to
> > make this work for exclude constraints too.
>
> Ok I think I see what's happening here.  We p-lock a page in
> t_item_id_created_at_key, but not t_item_id_version_key.  Then the UCV
> happens on t_item_id_version_key.  If you reverse the order of the
> UNIQUE clauses in the CREATE statement, you get a serialization
> failure instead.

New thought: can we use the recently invented errsave() mechanism for
this?  (Or some other error saving/deferring/rethrowing technology.)
The goal is to give UCV a lower priority than SSI violation, across
all indexes involved.  So perhaps the ereport(ERROR, ... "duplicate
key value violates ...") in nbtinsert.c could become an errsave()?
Then ExecInsertIndexTuples() could loop over all inserts, giving them
all a chance to raise an SSI error, and possibly collect up to one
deferred UCV.  If we make it out of the indexing loop because no SSI
(or other) error was thrown, we can then throw the UCV error, if we
collected one.  But to use errsave() I suppose we'd have to pass the
ErrorSaveContext via the index AM interface...

On Thu, May 21, 2020 at 9:24 AM Benny Kramek <benny(at)medflyt(dot)com> wrote:
> Hello, I have found an example where I expect to get a serialization failure
error, but instead receive a unique constraint error. My understanding is that
this is not supposed to happen starting with PostgreSQL version 9.6 because of
this patch:
>
> <https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fcff8a575198478023ada8a48e13b50f70054766>

I'm also running into this bug (with both Postgres 13.7 and 15.1) and am
wondering if there's any progress on this bug, or if there is a known
workaround for it.

After explaining what I saw on IRC, "ysch" (Yaroslav Schekin) pointed
me to this wiki page: https://wiki.postgresql.org/wiki/SerializableToDo
and I was able to write a test case in my own code base to confirm this
is what I'm running into.

While fiddling with my tests, I noticed that when you REINDEX _either_
of the indexes in the testcase before starting the transactions, somehow
a serialization error is triggered instead of a uniqueness constraint
error!  I verified that this also happens with the minimal testcase
provided by OP.

Of course this is not exactly a useful workaround for a production db,
but maybe it helps in finding out why this is failing the way it is?

Cheers,
Peter Bex