Re: Adding a test for speculative insert abort case

Hi,

On 2019-06-05 15:49:47 -0700, Melanie Plageman wrote:
> On Thu, May 16, 2019 at 8:46 PM Melanie Plageman <melanieplageman(at)gmail(dot)com>
> wrote:
>
> >
> > Good idea.
> > I squashed the changes I suggested in previous emails, Ashwin's patch, my
> > suggested updates to that patch, and the index order check all into one
> > updated
> > patch attached.
> >
> >
> I've updated this patch to make it apply on master cleanly. Thanks to
> Alvaro for format-patch suggestion.

Planning to push this, now that v12 is branched off. But only to master, I
don't think it's worth backpatching at the moment.

> The second patch in the set is another suggestion I have. I noticed
> that the insert-conflict-toast test mentions that it is "not
> guaranteed to lead to a failed speculative insertion" and, since it
> seems to be testing the speculative abort but with TOAST tables, I
> thought it might work to kill that spec file and move that test case
> into insert-conflict-specconflict so the test can utilize the existing
> advisory locks being used for the other tests in that file to make it
> deterministic which session succeeds in inserting the tuple.

Seems like a good plan.
> 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 ...

> +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.

> + # 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
+

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.

Greetings,

Andres Freund