Re: Adding a test for speculative insert abort case

On Wed, Aug 7, 2019 at 1:47 PM Melanie Plageman <melanieplageman(at)gmail(dot)com>
wrote:

>
>
> 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())
>

So, Taylor and I had hoped to rename the steps to something more specific
that
told the story of what this test is doing and made it more clear.
Unfortunately,
our attempt to do that didn't work and made step re-use very difficult.
Alas, we decided the original names were less confusing.

My idea for renaming blurt_and_lock2() was actually to rename
blurt_and_lock()
to blurt_and_lock_123() -- since it always takes a lock on 1,2, or 3.
Then, I could name the second function, which locks 4, blurt_and_lock_4().
What do you think?

I've attached a rebased patch updated with the new function names.

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

So, what should we do about this? Do you agree that the "controller_show"
step
would be a problem?

--
Melanie Plageman