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@gmail.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