Re: Adding a test for speculative insert abort case

On Wed, Jul 24, 2019 at 11:48 AM Andres Freund <andres@anarazel.de> wrote:

> diff --git a/src/test/isolation/specs/insert-conflict-specconflict.spec b/src/test/isolation/specs/insert-conflict-specconflict.spec
> index 3a70484fc2..7f29fb9d02 100644
> --- a/src/test/isolation/specs/insert-conflict-specconflict.spec
> +++ b/src/test/isolation/specs/insert-conflict-specconflict.spec
> @@ -10,7 +10,7 @@ setup
>  {
>       CREATE OR REPLACE FUNCTION blurt_and_lock(text) RETURNS text IMMUTABLE LANGUAGE plpgsql AS $$
>       BEGIN
> -        RAISE NOTICE 'called for %', $1;
> +        RAISE NOTICE 'blurt_and_lock() called for %', $1;
> 
>       -- depending on lock state, wait for lock 2 or 3
>          IF pg_try_advisory_xact_lock(current_setting('spec.session')::int, 1) THEN
> @@ -23,9 +23,16 @@ setup
>      RETURN $1;
>      END;$$;
> 
> +    CREATE OR REPLACE FUNCTION blurt_and_lock2(text) RETURNS text IMMUTABLE LANGUAGE plpgsql AS $$
> +    BEGIN
> +        RAISE NOTICE 'blurt_and_lock2() called for %', $1;
> +        PERFORM pg_advisory_xact_lock(current_setting('spec.session')::int, 4);
> +    RETURN $1;
> +    END;$$;
> +

Any chance for a bit more descriptive naming than *2? I can live with
it, but ...

Taylor Vesely and I paired on updating this test, and, it became clear
that the way that the steps and functions are named makes it very
difficult to understand what the test is doing. That is, I helped
write this test and, after a month away, I could no longer understand
what it was doing at all.

We changed the text of the blurts to "acquiring advisory lock ..."
from "blocking" because we realized that this would print even when
the lock was acquired immediately successfully, which is a little
misleading for the reader.

He's taking a stab at some renaming/refactoring to make it more clear
(including renaming blurt_and_lock2())

 

> +step "controller_print_speculative_locks" { SELECT locktype,classid,objid,mode,granted FROM pg_locks WHERE locktype='speculative
> +token' ORDER BY granted; }

I think showing the speculative locks is possibly going to be unreliable
- the release time of speculative locks is IIRC not that reliable. I
think it could e.g. happen that speculative locks are held longer
because autovacuum spawned an analyze in the background.

I actually think having the "controller_print_speculative_locks"
wouldn't be a problem because we have not released the advisory lock
on 4 in s2 that allows it to complete its speculative insertion and so
s1 will still be in speculative wait.

The step that might be a problem if autovacuum delays release of the
speculative locks is the "controller_show" step, because, at that
point, if the lock wasn't released, then s1 would still be waiting and
wouldn't have updated.