Adding a test for speculative insert abort case

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Adding a test for speculative insert abort case

Melanie Plageman
Today, while poking around the table_complete_speculative code which Ashwin
mentioned in [1], we were trying to understand when exactly we would complete a
speculative insert by aborting.

We added a logging message to heapam_tuple_complete_speculative before it calls
heap_abort_speculative and ran the regression and isolation tests to see what
test failed so we knew how to exercise this codepath.
No tests failed, so we spent some time trying to understand when 'succeeded'
would be true coming into heap_tuple_complete_speculative.

Eventually, we figured out that if one transaction speculatively inserts a
tuple into a table with a unique index and then pauses before inserting the
value into the index, and while it is paused, another transaction successfully
inserts a value which would conflict with that value, it would result in an
aborted speculative insertion.

t1(id,val)
unique index t1(id)

s1: insert into t1 values(1, 'someval') on conflict(id) do update set val = 'someotherval';
s1: pause in ExecInsert before calling ExecInsertIndexTuples
s2: insert into t1 values(1, 'someval');
s2: continue

We don't know of a way to add this scenario to the current isolation framework.

Can anyone think of a good way to put this codepath under test?

- Melanie & Ashwin

[1] https://www.postgresql.org/message-id/CALfoeitk7-TACwYv3hCw45FNPjkA86RfXg4iQ5kAOPhR%2BF1Y4w%40mail.gmail.com
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-04-30 17:15:55 -0700, Melanie Plageman wrote:
> Today, while poking around the table_complete_speculative code which Ashwin
> mentioned in [1], we were trying to understand when exactly we would
> complete a
> speculative insert by aborting.

(FWIW, it's on my todo queue to look at this)


> We added a logging message to heapam_tuple_complete_speculative before it
> calls
> heap_abort_speculative and ran the regression and isolation tests to see
> what
> test failed so we knew how to exercise this codepath.
> No tests failed, so we spent some time trying to understand when 'succeeded'
> would be true coming into heap_tuple_complete_speculative.
>
> Eventually, we figured out that if one transaction speculatively inserts a
> tuple into a table with a unique index and then pauses before inserting the
> value into the index, and while it is paused, another transaction
> successfully
> inserts a value which would conflict with that value, it would result in an
> aborted speculative insertion.
>
> t1(id,val)
> unique index t1(id)
>
> s1: insert into t1 values(1, 'someval') on conflict(id) do update set val =
> 'someotherval';
> s1: pause in ExecInsert before calling ExecInsertIndexTuples
> s2: insert into t1 values(1, 'someval');
> s2: continue
>
> We don't know of a way to add this scenario to the current isolation
> framework.

> Can anyone think of a good way to put this codepath under test?

Not easily so - that's why the ON CONFLICT patch didn't add code
coverage for it :(. I wonder if you could whip something up by having
another non-unique expression index, where the expression acquires a
advisory lock? If that advisory lock where previously acquired by
another session, that should allow to write a reliable isolation test?

Alternatively, as a fallback, there's a short pgbench test, I wonder if we
could just adapt that to use ON CONFLICT UPDATE?

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman

On Tue, Apr 30, 2019 at 5:22 PM Andres Freund <[hidden email]> wrote:

Not easily so - that's why the ON CONFLICT patch didn't add code
coverage for it :(. I wonder if you could whip something up by having
another non-unique expression index, where the expression acquires a
advisory lock? If that advisory lock where previously acquired by
another session, that should allow to write a reliable isolation test?


So, I took a look at one of the existing tests that does something like what
you mentioned and tried the following:
----------
create table t1(key int, val text);
create unique index t1_uniq_idx on t1(key);
create or replace function t1_lock_func(int) returns int immutable language sql AS
'select pg_advisory_xact_lock_shared(1); select $1';
create index t1_lock_idx ON t1(t1_lock_func(key));
----------
s1:
begin isolation level read committed;
insert into t1 values(1, 'someval');
s2:
set default_transaction_isolation = 'read committed';
insert into t1 values(1, 'anyval') on conflict(key) do update set val = 'updatedval';
----------

So, the above doesn't work because s2 waits to acquire the lock in the first
phase of the speculative insert -- when it is just checking the index, before
inserting to the table and before inserting to the index.

Then when the s1 is committed, we won't execute the speculative insert code at
all and will go into ExecOnConflictUpdate instead.

Maybe I just need a different kind of advisory lock to allow
ExecCheckIndexConstraints to be able to check the index here. I figured it is a
read operation, so a shared advisory lock should be okay, but it seems like it
is not okay

Without knowing any of the context, on an initial pass of debugging, I did
notice that, in the initial check of the index by s2, XactLockTableWait is
called with reason_wait as XLTW_InsertIndex (even though we are just trying to
check it, so maybe it knows our intentions:))

Is there something I can do in the test to allow my check to go
through but the insert to have to wait?
 
--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Peter Geoghegan-4
In reply to this post by Melanie Plageman
On Tue, Apr 30, 2019 at 5:16 PM Melanie Plageman
<[hidden email]> wrote:
> Can anyone think of a good way to put this codepath under test?

During the initial development of ON CONFLICT, speculative insertion
itself was tested using custom stress testing that you can still get
here:

https://github.com/petergeoghegan/jjanes_upsert

I'm not sure that this is something that you can adopt, but I
certainly found it very useful at the time. It tests whether or not
there is agreement among concurrent speculative inserters, and whether
or not there are "unprincipled deadlocks" (user hostile deadlocks that
cannot be fixed by reordering something in application code).

--
Peter Geoghegan


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On April 30, 2019 6:43:08 PM PDT, Peter Geoghegan <[hidden email]> wrote:

>On Tue, Apr 30, 2019 at 5:16 PM Melanie Plageman
><[hidden email]> wrote:
>> Can anyone think of a good way to put this codepath under test?
>
>During the initial development of ON CONFLICT, speculative insertion
>itself was tested using custom stress testing that you can still get
>here:
>
>https://github.com/petergeoghegan/jjanes_upsert
>
>I'm not sure that this is something that you can adopt, but I
>certainly found it very useful at the time. It tests whether or not
>there is agreement among concurrent speculative inserters, and whether
>or not there are "unprincipled deadlocks" (user hostile deadlocks that
>cannot be fixed by reordering something in application code).

I think we want a deterministic case. I recall asking for that back then...

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Thomas Munro-5
In reply to this post by Melanie Plageman
On Wed, May 1, 2019 at 12:16 PM Melanie Plageman
<[hidden email]> wrote:
> s1: insert into t1 values(1, 'someval') on conflict(id) do update set val = 'someotherval';
> s1: pause in ExecInsert before calling ExecInsertIndexTuples
> s2: insert into t1 values(1, 'someval');
> s2: continue
>
> We don't know of a way to add this scenario to the current isolation framework.
>
> Can anyone think of a good way to put this codepath under test?

Hi Melanie,

I think it'd be nice to have a set of macros that can create wait
points in the C code that isolation tests can control, in a special
build.  Perhaps there could be shm hash table of named wait points in
shared memory; if DEBUG_WAIT_POINT("foo") finds that "foo" is not
present, it continues, but if it finds an entry it waits for it to go
away.  Then isolation tests could add/remove names and signal a
condition variable to release waiters.

I contemplated that while working on SKIP LOCKED, which had a bunch of
weird edge cases that I tested by inserting throw-away wait-point code
like this:

https://www.postgresql.org/message-id/CADLWmXXss83oiYD0pn_SfQfg%2ByNEpPbPvgDb8w6Fh--jScSybA%40mail.gmail.com

--
Thomas Munro
https://enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman


On Tue, Apr 30, 2019 at 7:14 PM Thomas Munro <[hidden email]> wrote:
I think it'd be nice to have a set of macros that can create wait
points in the C code that isolation tests can control, in a special
build.  Perhaps there could be shm hash table of named wait points in
shared memory; if DEBUG_WAIT_POINT("foo") finds that "foo" is not
present, it continues, but if it finds an entry it waits for it to go
away.  Then isolation tests could add/remove names and signal a
condition variable to release waiters.

I contemplated that while working on SKIP LOCKED, which had a bunch of
weird edge cases that I tested by inserting throw-away wait-point code
like this:

https://www.postgresql.org/message-id/CADLWmXXss83oiYD0pn_SfQfg%2ByNEpPbPvgDb8w6Fh--jScSybA%40mail.gmail.com

Yes, I agree it would be nice to have a framework like this.

Greenplum actually has a fault injection framework that, I believe, works
similarly to what you are describing -- i.e. sets a variable in shared memory.
There is an extension, gp_inject_fault, which allows you to set the faults.
Andreas Scherbaum wrote a blog post about how to use it [1].

The Greenplum implementation is not documented particularly well in the code,
but, it is something that folks working on Greenplum have talked about modifying
and proposing to Postgres.

[1] http://engineering.pivotal.io/post/testing_greenplum_database_using_fault_injection/
 
--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
In reply to this post by Melanie Plageman
Hi,

On 2019-04-30 18:34:42 -0700, Melanie Plageman wrote:

> On Tue, Apr 30, 2019 at 5:22 PM Andres Freund <[hidden email]> wrote:
>
> >
> > Not easily so - that's why the ON CONFLICT patch didn't add code
> > coverage for it :(. I wonder if you could whip something up by having
> > another non-unique expression index, where the expression acquires a
> > advisory lock? If that advisory lock where previously acquired by
> > another session, that should allow to write a reliable isolation test?
> >
> >
> So, I took a look at one of the existing tests that does something like what
> you mentioned and tried the following:
> ----------
> create table t1(key int, val text);
> create unique index t1_uniq_idx on t1(key);
> create or replace function t1_lock_func(int) returns int immutable language
> sql AS
> 'select pg_advisory_xact_lock_shared(1); select $1';
> create index t1_lock_idx ON t1(t1_lock_func(key));
> ----------
> s1:
> begin isolation level read committed;
> insert into t1 values(1, 'someval');
> s2:
> set default_transaction_isolation = 'read committed';
> insert into t1 values(1, 'anyval') on conflict(key) do update set val =
> 'updatedval';
> ----------
>
> So, the above doesn't work because s2 waits to acquire the lock in the first
> phase of the speculative insert -- when it is just checking the index,
> before
> inserting to the table and before inserting to the index.

Couldn't that be addressed by having t1_lock_func() acquire two locks?
One for blocking during the initial index probe, and one for the
speculative insertion?

I'm imagining something like

if (pg_try_advisory_xact_lock(1))
    pg_advisory_xact_lock(2);
else
    pg_advisory_xact_lock(1);

in t1_lock_func. If you then make the session something roughly like

s1: pg_advisory_xact_lock(1);
s1: pg_advisory_xact_lock(2);

s2: upsert t1 <blocking for 1>
s1: pg_advisory_xact_unlock(1);
s2: <continuing>
s2: <blocking for 2>
s1: insert into t1 values(1, 'someval');
s1: pg_advisory_xact_unlock(2);
s2: <continuing>
s2: spec-conflict

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-05-01 11:41:48 -0700, Andres Freund wrote:

> I'm imagining something like
>
> if (pg_try_advisory_xact_lock(1))
>     pg_advisory_xact_lock(2);
> else
>     pg_advisory_xact_lock(1);
>
> in t1_lock_func. If you then make the session something roughly like
>
> s1: pg_advisory_xact_lock(1);
> s1: pg_advisory_xact_lock(2);
>
> s2: upsert t1 <blocking for 1>
> s1: pg_advisory_xact_unlock(1);
> s2: <continuing>
> s2: <blocking for 2>
> s1: insert into t1 values(1, 'someval');
> s1: pg_advisory_xact_unlock(2);
> s2: <continuing>
> s2: spec-conflict
Needed to be slightly more complicated than that, but not that much. See
the attached test.  What do you think?

I think we should apply something like this (minus the WARNING, of
course). It's a bit complicated, but it seems worth covering this
special case.

Greetings,

Andres Freund

speculative-conflict.diff (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-05-10 14:40:38 -0700, Andres Freund wrote:

> On 2019-05-01 11:41:48 -0700, Andres Freund wrote:
> > I'm imagining something like
> >
> > if (pg_try_advisory_xact_lock(1))
> >     pg_advisory_xact_lock(2);
> > else
> >     pg_advisory_xact_lock(1);
> >
> > in t1_lock_func. If you then make the session something roughly like
> >
> > s1: pg_advisory_xact_lock(1);
> > s1: pg_advisory_xact_lock(2);
> >
> > s2: upsert t1 <blocking for 1>
> > s1: pg_advisory_xact_unlock(1);
> > s2: <continuing>
> > s2: <blocking for 2>
> > s1: insert into t1 values(1, 'someval');
> > s1: pg_advisory_xact_unlock(2);
> > s2: <continuing>
> > s2: spec-conflict
>
> Needed to be slightly more complicated than that, but not that much. See
> the attached test.  What do you think?
>
> I think we should apply something like this (minus the WARNING, of
> course). It's a bit complicated, but it seems worth covering this
> special case.

And pushed. Let's see what the buildfarm says.

Regards,

Andres


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman


On Tue, May 14, 2019 at 12:19 PM Andres Freund <[hidden email]> wrote:
Hi,

On 2019-05-10 14:40:38 -0700, Andres Freund wrote:
> On 2019-05-01 11:41:48 -0700, Andres Freund wrote:
> > I'm imagining something like
> >
> > if (pg_try_advisory_xact_lock(1))
> >     pg_advisory_xact_lock(2);
> > else
> >     pg_advisory_xact_lock(1);
> >
> > in t1_lock_func. If you then make the session something roughly like
> >
> > s1: pg_advisory_xact_lock(1);
> > s1: pg_advisory_xact_lock(2);
> >
> > s2: upsert t1 <blocking for 1>
> > s1: pg_advisory_xact_unlock(1);
> > s2: <continuing>
> > s2: <blocking for 2>
> > s1: insert into t1 values(1, 'someval');
> > s1: pg_advisory_xact_unlock(2);
> > s2: <continuing>
> > s2: spec-conflict
>
> Needed to be slightly more complicated than that, but not that much. See
> the attached test.  What do you think?
>
> I think we should apply something like this (minus the WARNING, of
> course). It's a bit complicated, but it seems worth covering this
> special case.

And pushed. Let's see what the buildfarm says.

Regards,

Andres

So, I recognize this has already been merged. However, after reviewing the test,
I believe there is a typo in the second permutation.

# Test that speculative locks are correctly acquired and released, s2
# inserts, s1 updates.

I think you meant

# Test that speculative locks are correctly acquired and released, s1
# inserts, s2 updates.

Though, I'm actually not sure how this permutation is exercising different
code than the first permutation.

Also, it would make the test easier to understand for me if, for instances of the
word "lock" in the test description and comments, you specified locktype -- e.g.
advisory lock.
I got confused between the speculative lock, the object locks on the index which
are required for probing or inserting into the index, and the advisory locks.

Below is a potential re-wording of one of the permutations that is more explicit
and more clear to me as a reader.

# Test that speculative locks are correctly acquired and released, s2
# inserts, s1 updates.
permutation
   # acquire a number of advisory locks, to control execution flow - the
   # blurt_and_lock function acquires advisory locks that allow us to
   # continue after a) the optimistic conflict probe b) after the
   # insertion of the speculative tuple.

   "controller_locks"
   "controller_show"
   # Both sessions will be waiting on advisory locks
   "s1_upsert" "s2_upsert"
   "controller_show"
   # Switch both sessions to wait on the other advisory lock next time
   "controller_unlock_1_1" "controller_unlock_2_1"
   # Allow both sessions to do the optimistic conflict probe and do the
   # speculative insertion into the table
   # They will then be waiting on another advisory lock when they attempt to
   # update the index
   "controller_unlock_1_3" "controller_unlock_2_3"
   "controller_show"
   # Allow the second session to finish insertion (complete speculative)
   "controller_unlock_2_2"
   # This should now show a successful insertion
   "controller_show"
   # Allow the first session to finish insertion (abort speculative)
   "controller_unlock_1_2"
   # This should now show a successful UPSERT
   "controller_show"

I was also wondering: Is it possible that one of the "controller_unlock_*"
functions will get called before the session with the upsert has had a chance to
move forward in its progress and be waiting on that lock?
That is, given that we don't check that the sessions are waiting on the locks
before unlocking them, is there a race condition?

I noticed that there is not a test case which would cover the speculative wait
codepath. This seems much more challenging, however, it does seem like a
worthwhile test to have.

--
Melanie Plageman
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-05-15 18:34:15 -0700, Melanie Plageman wrote:

> So, I recognize this has already been merged. However, after reviewing the
> test,
> I believe there is a typo in the second permutation.
>
> # Test that speculative locks are correctly acquired and released, s2
> # inserts, s1 updates.
>
> I think you meant
>
> # Test that speculative locks are correctly acquired and released, s1
> # inserts, s2 updates.

Hm, yea.


> Though, I'm actually not sure how this permutation is exercising differen.
> code than the first permutation.

I was basically just trying to make sure that there's a sensible result
independent of which transaction "wins", while keeping the command-start
order the same. Probably not crucial, but seemed like a reasonable
addition.


> Also, it would make the test easier to understand for me if, for instances
> of the
> word "lock" in the test description and comments, you specified locktype --
> e.g.
> advisory lock.
> I got confused between the speculative lock, the object locks on the index
> which
> are required for probing or inserting into the index, and the advisory
> locks.
>
> Below is a potential re-wording of one of the permutations that is more
> explicit
> and more clear to me as a reader.

Minor gripe: For the future, it's easier to such changes as a patch as
well - otherwise others need to move it to the file and diff it to
comment on the changes.


> # Test that speculative locks are correctly acquired and released, s2
> # inserts, s1 updates.
> permutation
>    # acquire a number of advisory locks, to control execution flow - the
>    # blurt_and_lock function acquires advisory locks that allow us to
>    # continue after a) the optimistic conflict probe b) after the
>    # insertion of the speculative tuple.
>
>    "controller_locks"
>    "controller_show"
>    # Both sessions will be waiting on advisory locks
>    "s1_upsert" "s2_upsert"
>    "controller_show"
>    # Switch both sessions to wait on the other advisory lock next time
>    "controller_unlock_1_1" "controller_unlock_2_1"
>    # Allow both sessions to do the optimistic conflict probe and do the
>    # speculative insertion into the table
>    # They will then be waiting on another advisory lock when they attempt to
>    # update the index
>    "controller_unlock_1_3" "controller_unlock_2_3"
>    "controller_show"
>    # Allow the second session to finish insertion (complete speculative)
>    "controller_unlock_2_2"
>    # This should now show a successful insertion
>    "controller_show"
>    # Allow the first session to finish insertion (abort speculative)
>    "controller_unlock_1_2"
>    # This should now show a successful UPSERT
>    "controller_show"


> I was also wondering: Is it possible that one of the
> "controller_unlock_*" functions will get called before the session
> with the upsert has had a chance to move forward in its progress and
> be waiting on that lock?  That is, given that we don't check that the
> sessions are waiting on the locks before unlocking them, is there a
> race condition?

Isolationtester only switches between commands when either the command
finished, or once it's know to be waiting for a lock. Therefore I don't
think this race exists?  That logic is in the if (flags & STEP_NONBLOCK)
block in isolationtester.c:try_complete_step().

Does that make sense? Or did I misunderstand your concern?


> I noticed that there is not a test case which would cover the speculative
> wait
> codepath. This seems much more challenging, however, it does seem like a
> worthwhile test to have.

Shouldn't be that hard to create, I think. I think acquiring another
lock in a second, non-unique, expression index, ought to do the trick?
It probably has to be created after the unique index (so it's later in
the

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman


On Wed, May 15, 2019 at 6:50 PM Andres Freund <[hidden email]> wrote:
> Also, it would make the test easier to understand for me if, for instances
> of the
> word "lock" in the test description and comments, you specified locktype --
> e.g.
> advisory lock.
> I got confused between the speculative lock, the object locks on the index
> which
> are required for probing or inserting into the index, and the advisory
> locks.
>
> Below is a potential re-wording of one of the permutations that is more
> explicit
> and more clear to me as a reader.

Minor gripe: For the future, it's easier to such changes as a patch as
well - otherwise others need to move it to the file and diff it to
comment on the changes.

 
Will do--attached, though the wording is a rough suggestion.

> I was also wondering: Is it possible that one of the
> "controller_unlock_*" functions will get called before the session
> with the upsert has had a chance to move forward in its progress and
> be waiting on that lock?  That is, given that we don't check that the
> sessions are waiting on the locks before unlocking them, is there a
> race condition?

Isolationtester only switches between commands when either the command
finished, or once it's know to be waiting for a lock. Therefore I don't
think this race exists?  That logic is in the if (flags & STEP_NONBLOCK)
block in isolationtester.c:try_complete_step().

Does that make sense? Or did I misunderstand your concern?


I see. I didn't know what the blocking/waiting logic was in the isolation
framework. Nevermind, then.
 

> I noticed that there is not a test case which would cover the speculative
> wait
> codepath. This seems much more challenging, however, it does seem like a
> worthwhile test to have.

Shouldn't be that hard to create, I think. I think acquiring another
lock in a second, non-unique, expression index, ought to do the trick?
It probably has to be created after the unique index (so it's later in
the


I would think that the sequence would be s1 and s2 probe the index, s1 and s2
insert into the table, s1 updates the index but does not complete the
speculative insert and clear the token (pause before
table_complete_speculative). s2 is in speculative wait when attempting to update
the index.

Something like

permutation
   "controller_locks"
   "controller_show"
   "s1_upsert" "s2_upsert"
   "controller_show"
   "controller_unlock_1_1" "controller_unlock_2_1"
   "controller_unlock_1_3" "controller_unlock_2_3"
   "controller_unlock_1_2"
   "s1_magically_pause_before_complete_speculative"
   # put s2 in speculative wait
   "controller_unlock_2_2"
   "s1_magically_unpause_before_complete_speculative"

So, how would another lock on another index keep s1 from clearing the
speculative token after it has updated the index?

--
Melanie Plageman

specconflict_permutation_comment_update.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-05-15 20:35:49 -0700, Melanie Plageman wrote:

> > > I noticed that there is not a test case which would cover the speculative
> > > wait
> > > codepath. This seems much more challenging, however, it does seem like a
> > > worthwhile test to have.
> >
> > Shouldn't be that hard to create, I think. I think acquiring another
> > lock in a second, non-unique, expression index, ought to do the trick?
> > It probably has to be created after the unique index (so it's later in
> > the
> >
> >
> I would think that the sequence would be s1 and s2 probe the index, s1 and
> s2
> insert into the table, s1 updates the index but does not complete the
> speculative insert and clear the token (pause before
> table_complete_speculative). s2 is in speculative wait when attempting to
> update
> the index.
>
> Something like
>
> permutation
>    "controller_locks"
>    "controller_show"
>    "s1_upsert" "s2_upsert"
>    "controller_show"
>    "controller_unlock_1_1" "controller_unlock_2_1"
>    "controller_unlock_1_3" "controller_unlock_2_3"
>    "controller_unlock_1_2"
>    "s1_magically_pause_before_complete_speculative"
>    # put s2 in speculative wait
>    "controller_unlock_2_2"
>    "s1_magically_unpause_before_complete_speculative"
>
> So, how would another lock on another index keep s1 from clearing the
> speculative token after it has updated the index?

If there were a second index on upserttest, something like CREATE INDEX
ON upserttest((blurt_and_lock2(key))); and blurt_and_lock2 acquired a
lock on (current_setting('spec.session')::int, 4), ISTM you could cause
a block to happen after the first index (the unique one, used for ON
CONFLICT) successfully created the index entry, but before
complete_speculative is called.  Shouldn't that fulfil your
s1_magically_pause_before_complete_speculative goal? The
controller_locks would only acquire the (1, 4) lock, thereby *not*
blocking s2 (or you could just release the lock in a separate step).

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Ashwin Agrawal
In reply to this post by Melanie Plageman

On Wed, May 15, 2019 at 8:36 PM Melanie Plageman <[hidden email]> wrote:

On Wed, May 15, 2019 at 6:50 PM Andres Freund <[hidden email]> wrote:

> I noticed that there is not a test case which would cover the speculative
> wait
> codepath. This seems much more challenging, however, it does seem like a
> worthwhile test to have.

Shouldn't be that hard to create, I think. I think acquiring another
lock in a second, non-unique, expression index, ought to do the trick?
It probably has to be created after the unique index (so it's later in
the

I would think that the sequence would be s1 and s2 probe the index, s1 and s2
insert into the table, s1 updates the index but does not complete the
speculative insert and clear the token (pause before
table_complete_speculative). s2 is in speculative wait when attempting to update
the index.

Something like

permutation
   "controller_locks"
   "controller_show"
   "s1_upsert" "s2_upsert"
   "controller_show"
   "controller_unlock_1_1" "controller_unlock_2_1"
   "controller_unlock_1_3" "controller_unlock_2_3"
   "controller_unlock_1_2"
   "s1_magically_pause_before_complete_speculative"
   # put s2 in speculative wait
   "controller_unlock_2_2"
   "s1_magically_unpause_before_complete_speculative"

So, how would another lock on another index keep s1 from clearing the
speculative token after it has updated the index?

The second index would help to hold the session after inserting the tuple in unique index but before completing the speculative insert. Hence, helps to create the condition easily. I believe order of index insertion is helping here that unique index is inserted and then non-unique index is inserted too.

Attaching patch with the test using the idea Andres mentioned and it works to excercise the speculative wait.


0001-Add-test-to-validate-speculative-wait-is-performed.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman


On Wed, May 15, 2019 at 10:32 PM Ashwin Agrawal <[hidden email]> wrote:

The second index would help to hold the session after inserting the tuple in unique index but before completing the speculative insert. Hence, helps to create the condition easily. I believe order of index insertion is helping here that unique index is inserted and then non-unique index is inserted too.


Oh, cool. I didn't know that execution order would be guaranteed for which index
to insert into first.
 
Attaching patch with the test using the idea Andres mentioned and it works to excercise the speculative wait.


It looks good.
I thought it would be helpful to mention why you have s1 create the non-unique
index after the permutation has begun. You don't want this index to influence
the behavior of the other permutations--this part makes sense. However, why have
s1 do it instead of the controller?

I added a couple suggested changes to the comments in the permutation in the
version in the patch I attached. Note that I did not update the answer files.
(These suggested changes to comments are in distinct from and would be in
addition to the suggestions I had for the wording of the comments overall in the
above email I sent).

--
Melanie Plageman

0002-Add-test-to-validate-speculative-wait-is-performed.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Andres Freund
Hi,

On 2019-05-16 13:59:47 -0700, Melanie Plageman wrote:

> On Wed, May 15, 2019 at 10:32 PM Ashwin Agrawal <[hidden email]> wrote:
>
> >
> > The second index would help to hold the session after inserting the tuple
> > in unique index but before completing the speculative insert. Hence, helps
> > to create the condition easily. I believe order of index insertion is
> > helping here that unique index is inserted and then non-unique index is
> > inserted too.
> >
> >
> Oh, cool. I didn't know that execution order would be guaranteed for which
> index
> to insert into first.

It's not *strictly* speaking *always* well defined. The list of indexes
is sorted by the oid of the index - so once created, it's
consistent. But when the oid assignment wraps around, it'd be the other
way around. But I think it's ok to disregard that - it'll never happen
in regression tests run against a new cluster, and you'd have to run
tests against an installed cluster for a *LONG* time for a *tiny* window
where the wraparound would happen precisely between the creation of the
two indexes.

Makes sense?

I guess we could make that case a tiny bit easier to diagnose in the
extremely unlikely case it happens by having a step that outputs
SELECT 'index_a'::regclass::int8 < 'index_b'::regclass::int8;

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman


On Thu, May 16, 2019 at 2:03 PM Andres Freund <[hidden email]> wrote:
Hi,

On 2019-05-16 13:59:47 -0700, Melanie Plageman wrote:
> On Wed, May 15, 2019 at 10:32 PM Ashwin Agrawal <[hidden email]> wrote:
>
> >
> > The second index would help to hold the session after inserting the tuple
> > in unique index but before completing the speculative insert. Hence, helps
> > to create the condition easily. I believe order of index insertion is
> > helping here that unique index is inserted and then non-unique index is
> > inserted too.
> >
> >
> Oh, cool. I didn't know that execution order would be guaranteed for which
> index
> to insert into first.

It's not *strictly* speaking *always* well defined. The list of indexes
is sorted by the oid of the index - so once created, it's
consistent. But when the oid assignment wraps around, it'd be the other
way around. But I think it's ok to disregard that - it'll never happen
in regression tests run against a new cluster, and you'd have to run
tests against an installed cluster for a *LONG* time for a *tiny* window
where the wraparound would happen precisely between the creation of the
two indexes.

Makes sense?

Yep, thanks.


I guess we could make that case a tiny bit easier to diagnose in the
extremely unlikely case it happens by having a step that outputs
SELECT 'index_a'::regclass::int8 < 'index_b'::regclass::int8;


Good idea.
I squashed the changes I suggested in previous emails, Ashwin's patch, my
suggested updates to that patch, and the index order check all into one updated
patch attached.

--
Melanie Plageman

0003-Add-test-to-validate-speculative-wait-is-performed.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Melanie Plageman

On Thu, May 16, 2019 at 8:46 PM Melanie Plageman <[hidden email]> wrote:

I squashed the changes I suggested in previous emails, Ashwin's patch, my
suggested updates to that patch, and the index order check all into one updated
patch attached.


I realized that the numbers at the front probably indicate which patch it is in
a patch set and not the version, so, if that is the case, a renamed patch --
second version but the only patch needed if you are applying to master.
Is this right?

--
Melanie Plageman

0001-Add-test-to-validate-speculative-wait-is-performed-v2.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Adding a test for speculative insert abort case

Alvaro Herrera-9
On 2019-May-17, Melanie Plageman wrote:

> I realized that the numbers at the front probably indicate which patch
> it is in a patch set and not the version, so, if that is the case, a
> renamed patch -- second version but the only patch needed if you are
> applying to master.  Is this right?

That's correct.  I suggest that "git format-patch -vN origin/master",
where the N is the version you're currently posting, generates good
patch files to attach in email.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services