Thread: The drop-index-concurrently-1 isolation test no longer tests what it was meant to
The drop-index-concurrently-1 isolation test no longer tests what it was meant to
From
David Rowley
Date:
I'm in the middle of working on making some adjustments to the costs
of Incremental Sorts and I see the patch I wrote changes the plan in
the drop-index-concurrently-1 isolation test.
The particular plan changed currently expects:
-----------------------------------------------
Sort
Sort Key: id, data
-> Index Scan using test_dc_pkey on test_dc
Filter: ((data)::text = '34'::text)
It seems fairly clear from reading the test spec that this plan is
really meant to be a seq scan plan and the change made in [1] adjusted
that without any regard for that.
That seems to have come around because of how the path generation of
incremental sorts work. The current incremental sort path generation
will put a Sort path atop of the cheapest input path, even if that
cheapest input path has presorted keys. The test_dc_pkey index
provides presorted input for the required sort order. Prior to
incremental sort, we did not consider paths which only provided
presorted input to be useful paths, hence we used to get a seq scan
plan.
I propose the attached which gets rid of the not-so-great casting
method that was originally added to this test to try and force the seq
scan. It seems a little dangerous to put in hacks like that to force
a particular plan when the resulting plan ends up penalized with a
(1.0e10) disable_cost. The planner is just not going to be stable
when the plan includes such a large penalty. To force the planner,
I've added another test step to do set enable_seqscan to true and
adjusted the permutations to run that just before preparing the seq
scan query.
I also tried to make it more clear that we want to be running the
query twice, once with an index scan and again with a seq scan. I'm
hoping the changes to the prepared query names and the extra comments
will help reduce the chances of this getting broken again in the
future.
David
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blobdiff;f=src/test/isolation/expected/drop-index-concurrently-1.out;h=8e6adb66bb1479b8d7db2fcf5f70b89acd3af577;hp=75dff56bc46d40aa8eb012543044b7c10d516b7e;hb=d2d8a229bc5;hpb=3c8553547b1493c4afdb80393f4a47dbfa019a79
Attachment
Re: The drop-index-concurrently-1 isolation test no longer tests what it was meant to
From
David Rowley
Date:
On Thu, 15 Dec 2022 at 18:26, David Rowley <dgrowleyml@gmail.com> wrote: > I propose the attached which gets rid of the not-so-great casting > method that was originally added to this test to try and force the seq > scan. It seems a little dangerous to put in hacks like that to force > a particular plan when the resulting plan ends up penalized with a > (1.0e10) disable_cost. The planner is just not going to be stable > when the plan includes such a large penalty. To force the planner, > I've added another test step to do set enable_seqscan to true and > adjusted the permutations to run that just before preparing the seq > scan query. Pushed and backpatched to 13, where incremental sorts were added. David