Re: Improving the isolationtester: fewer failures, less delay

Hi,

Only halfway related: I wonder if we should remove the automatic
permutation stuff - it's practically never useful. Probably not worth
changing...


On 2021-06-15 17:09:00 -0400, Tom Lane wrote:
> +The general form of a permutation entry is
> +
> +	"step_name" [ ( marker [ , marker ... ] ) ]
> +
> +where each marker defines a "blocking condition".  The step will not be
> +reported as completed before all the blocking conditions are satisfied.

Minor suggestion: I think the folliwing would be a bit easier to read if
there first were a list of markers, and then separately the longer
descriptions. Right now it's a bit hard to see which paragraph
introduces a new type of marker, and which just adds further commentary.


> +				/*
> +				 * Check for other steps that have finished.  We must do this
> +				 * if oldstep completed; while if it did not, we need to poll
> +				 * all the active steps in hopes of unblocking oldstep.
> +				 */

Somehow I found the second sentence a bit hard to parse - I think it's
the "while ..." sounding a bit odd to me.


> +				/*
> +				 * If the target session is still busy, apply a timeout to
> +				 * keep from hanging indefinitely, which could happen with
> +				 * incorrect blocker annotations.  Use the same 2 *
> +				 * max_step_wait limit as try_complete_step does for deciding
> +				 * to die.  (We don't bother with trying to cancel anything,
> +				 * since it's unclear what to cancel in this case.)
> +				 */
> +				if (iconn->active_step != NULL)
> +				{
> +					struct timeval current_time;
> +					int64		td;
> +
> +					gettimeofday(&current_time, NULL);
> +					td = (int64) current_time.tv_sec - (int64) start_time.tv_sec;
> +					td *= USECS_PER_SEC;
> +					td += (int64) current_time.tv_usec - (int64) start_time.tv_usec;
>+					if (td > 2 * max_step_wait)
> +					{
> +						fprintf(stderr, "step %s timed out after %d seconds\n",
> +								iconn->active_step->name,
> +								(int) (td / USECS_PER_SEC));
> +						exit(1);
> +					}
> +				}
> +			}
>  		}

It might be worth printing out the list of steps the being waited for
when reaching the timeout - it seems like it'd now be easier to end up
with a bit hard to debug specs. And one cannot necessarily look at
pg_locks or such anymore to debug em.


> @@ -833,6 +946,19 @@ try_complete_step(TestSpec *testspec, Step *step, int flags)
>  		}
>  	}
>  
> +	/*
> +	 * The step is done, but we won't report it as complete so long as there
> +	 * are blockers.
> +	 */
> +	if (step_has_blocker(pstep))
> +	{
> +		if (!(flags & STEP_RETRY))
> +			printf("step %s: %s <waiting ...>\n",
> +				   step->name, step->sql);
> +		return true;
> +	}

Might be a bug in my mental state machine: Will this work correctly for
PSB_ONCE, where we'll already returned before?


Greetings,

Andres Freund