Re: tuplesort test coverage

Hi,

On 2019-12-12 09:27:04 -0500, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I pushed this now. We'll see what the slower buildfarm animals say. I'll
> > try to see how long they took in a few days.
>
> friarbird (a CLOBBER_CACHE_ALWAYS animal) just showed a failure in this:
>
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=friarbird&dt=2019-12-12%2006%3A20%3A02
>
> ================== pgsql.build/src/test/regress/regression.diffs ===================
> diff -U3 /pgbuild/root/HEAD/pgsql.build/../pgsql/src/test/regress/expected/tuplesort.out /pgbuild/root/HEAD/pgsql.build/src/test/regress/results/tuplesort.out
> --- /pgbuild/root/HEAD/pgsql.build/../pgsql/src/test/regress/expected/tuplesort.out 2019-11-13 19:54:11.000000000 -0500
> +++ /pgbuild/root/HEAD/pgsql.build/src/test/regress/results/tuplesort.out 2019-12-12 08:25:23.000000000 -0500
> @@ -625,13 +625,13 @@
> Group Key: a.col12
> Filter: (count(*) > 1)
> -> Merge Join
> - Merge Cond: (a.col12 = b.col12)
> - -> Sort
> - Sort Key: a.col12 DESC
> - -> Seq Scan on test_mark_restore a
> + Merge Cond: (b.col12 = a.col12)
> -> Sort
> Sort Key: b.col12 DESC
> -> Seq Scan on test_mark_restore b
> + -> Sort
> + Sort Key: a.col12 DESC
> + -> Seq Scan on test_mark_restore a
> (14 rows)
>
> :qry;
>
> Since a and b are exactly the same table, in principle it's a matter of
> chance which one the planner will put on the outside of the join.

Yea.

> I think what happened here is that the test ran long enough for
> autovacuum/autoanalyze to come along and scan the table, changing its
> stats in between where the planner picked up the stats for a and those
> for b, and we ended up making the opposite join order choice.

Sounds reasonable.

> What seems like a simpler and more reliable fix is to make
> test_mark_restore a temp table, thus keeping autovac away from it.
> Is there a reason in terms of the test's goals not to do that?

I can't see any reason. The sorting code shouldn't care about the source
of tuples. I guess there could at some point be tests for parallel
sorting, but that'd just use a different table.

> Also ... why in the world does the script drop its tables at the end
> with IF EXISTS? They'd better exist at that point. I object
> to the DROP IF EXISTS up at the top, too. The regression tests
> do not need to be designed to deal with an unpredictable start state,
> and coding them to do so can have no effect other than possibly
> masking problems.

Well, it makes it a heck of a lot easier to run tests in isolation while
evolving them. While I personally think it's good to leave cleanup for
partial states in for cases where it was helpful during development, I
also don't care about it strongly.

Greetings,

Andres Freund