Re: Adding a test for speculative insert abort case

On Tue, May 14, 2019 at 12:19 PM Andres Freund <andres(at)anarazel(dot)de> 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