Re: Adding a test for speculative insert abort case

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