Re: Adding a test for speculative insert abort case - Mailing list pgsql-hackers

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