Thread: serializable transaction: exclude constraint violation (backed byGIST index) instead of ssi conflict

On Thu, Apr 11, 2019 at 9:43 AM Peter Billen <peter.billen@gmail.com> wrote:
> I understood that v11 includes predicate locking for gist indexes, as per
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3ad55863e9392bff73377911ebbf9760027ed405.
>
> I tried this in combination with an exclude constraint as following:
>
> drop table if exists t;
> create table t(period tsrange);
> alter table t add constraint bla exclude using gist(period with &&);
> -- t1
> begin transaction isolation level serializable;
> select * from t where period && tsrange(now()::timestamp, now()::timestamp + interval '1 hour');
> insert into t(period) values(tsrange(now()::timestamp, now()::timestamp + interval '1 hour'));
> -- t2
> begin transaction isolation level serializable;
> select * from t where period && tsrange(now()::timestamp, now()::timestamp + interval '1 hour');
> insert into t(period) values(tsrange(now()::timestamp, now()::timestamp + interval '1 hour'));
> -- t1
> commit;
> -- t2
> ERROR:  conflicting key value violates exclusion constraint "bla"
> DETAIL:  Key (period)=(["2019-04-10 20:59:20.6265","2019-04-10 21:59:20.6265")) conflicts with existing key
(period)=(["2019-04-1020:59:13.332622","2019-04-10 21:59:13.332622")). 
>
> I kinda expected/hoped that transaction t2 would get aborted by a serialization error, and not an exclude constraint
violation.This makes the application session bound to transaction t2 failing, as only serialization errors are retried. 
>
> We introduced the same kind of improvement/fix for btree indexes earlier, see
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fcff8a575198478023ada8a48e13b50f70054766.Should this
alsobe applied for (exclude) constraints backed by a gist index (as gist indexes now support predicate locking), or am
Icreating incorrect assumptions something here? 

Hi Peter,

Yeah, I agree, the behaviour you are expecting is desirable and we
should figure out how to do that.  The basic trick for btree unique
constraints was to figure out where the index *would* have written, to
give the SSI machinery a chance to object to that before raising the
UCV.  I wonder if we can use the same technique here... at first
glance, check_exclusion_or_unique_constraint() is raising the error,
but is not index AM specific code, and it is somewhat removed from the
GIST code that would do the equivalent
CheckForSerializableConflictIn() call.  I haven't looked into it
properly, but that certainly complicates matters somewhat...  Perhaps
the index AM would actually need a new entrypoint that could be called
before the error is raised, or perhaps there is an easier way.

--
Thomas Munro
https://enterprisedb.com

On Thu, Apr 11, 2019 at 10:54 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Thu, Apr 11, 2019 at 9:43 AM Peter Billen <peter.billen@gmail.com> wrote:
> > I kinda expected/hoped that transaction t2 would get aborted by a serialization error, and not an exclude
constraintviolation. This makes the application session bound to transaction t2 failing, as only serialization errors
areretried.
 

> Yeah, I agree, the behaviour you are expecting is desirable and we
> should figure out how to do that.  The basic trick for btree unique
> constraints was to figure out where the index *would* have written, to
> give the SSI machinery a chance to object to that before raising the
> UCV.  I wonder if we can use the same technique here... at first
> glance, check_exclusion_or_unique_constraint() is raising the error,
> but is not index AM specific code, and it is somewhat removed from the
> GIST code that would do the equivalent
> CheckForSerializableConflictIn() call.  I haven't looked into it
> properly, but that certainly complicates matters somewhat...  Perhaps
> the index AM would actually need a new entrypoint that could be called
> before the error is raised, or perhaps there is an easier way.

Adding Kevin (architect of SSI and reviewer/committer of my UCV
interception patch) and Shubham (author of GIST SSI support) to the CC
list in case they have thoughts on this.

-- 
Thomas Munro
https://enterprisedb.com

On Fri, Apr 12, 2019 at 6:01 AM Peter Billen <peter.billen@gmail.com> wrote:
> On Thu, Apr 11, 2019 at 6:12 PM Peter Billen <peter.billen@gmail.com> wrote:
>> I believe we are after multiple issues/improvements:
>>
>> 1. Could we extend
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=fcff8a575198478023ada8a48e13b50f70054766by adding
supportfor exclude constraints? 
>> 2. Fully support gist & constraints in serializable transactions. I did not yet test a unique constraint backed by a
gistconstraint, which is also interesting to test I assume. This test would tell us if there currently is a status quo
betweenbtree and gist indexes. 
>
> Regarding the remark in (2), I forgot that a unique constraint cannot be backed by a gist index, so forget the test I
mentioned.

Yeah, well we can't directly extend the existing work because unique
constraints are *entirely* handled inside the btree code (in fact no
other index types even support unique constraints, yet).  This
exclusion constraints stuff is handled differently: the error handling
you're seeing is raised by generic code in
src/backend/executor/execIndexing.c , but the code that knows how to
actually perform the necessary SSI checks is index-specific, in this
case in gist.c.  To do the moral equivalent of the UCV change, we'll
need to get these two bits of code to communicate across the "index
AM" boundary (the way that index implementations such as GIST are
plugged into Postgres).  The question is how.

One (bad) idea is that we could actually perform the (illegal)
aminsert just before we raise that error!  We know we're going to roll
back anyway, because that's either going to fail when gist.c calls
CheckForSerializableConflictIn(), or if not, when we raise
"conflicting key value violates exclusion constraint ...".  That's a
bit messy though, because it modifies the index unnecessarily and
possibly breaks important invariants.  An improved version of that
idea is to add a new optional index AM interface "amcheckinsert()"
that shares whatever code it needs to share to do all the work that
insert would do except the actual modification.  That way,
check_exclusion_or_unique_constraint() would give every index AM a
chance to raise an SSI error if it wants to.  This seems like it
should work, but I don't want to propose messing around with the index
AM interface lightly.  It wouldn't usually get called, just in the
error path.

Anyone got a better idea?

--
Thomas Munro
https://enterprisedb.com