Re: Adding a test for speculative insert abort case

On Wed, Jul 24, 2019 at 11:48 AM Andres Freund <andres(at)anarazel(dot)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.

>
> > + # Should report s1 is waiting on speculative lock
> > + "controller_print_speculative_locks"
>
> Hm, I might be missing something, but I don't think it currently
> does. Looking at the expected file:
>

+step controller_print_speculative_locks: SELECT
> locktype,classid,objid,mode,granted FROM pg_locks WHERE
> locktype='speculative
> +token' ORDER BY granted;
> +locktype classid objid mode granted
>
> +
>
>
Oops! due to an errant newline, the query wasn't correct.

> And if it showed something, it'd make the test not work, because
> classid/objid aren't necessarily going to be the same from test to test.
>
>
Good point. In the attached patch, classid/objid columns are removed
from the SELECT list.

Melanie & Taylor