Re: BUG #16122: segfault pg_detoast_datum (datum=0x0) at fmgr.c:1833 numrange query
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Michael Paquier <michael@paquier.xyz>,
Tomas Vondra <tomas.vondra@2ndquadrant.com>,
Adam Scott <adam.c.scott@gmail.com>, pgsql-bugs@lists.postgresql.org
Date: 2020-01-06T20:28:34Z
Lists: pgsql-bugs
Attachments
- bug-16122-fix-wip.patch (text/x-diff) patch
Andrey Borodin <x4mmm@yandex-team.ru> writes: > PFA possible fix for this. I looked into this a bit too. I don't have a lot of faith in Michael's example; it took me four tries to find a machine it would fail on. Still, I did eventually get a crash, and I concur that the problem is that calc_hist_selectivity_contained tries to access the entry after the histogram end. I believe that Michael's draft fix is actually about right, because we don't want to treat the key space above the histogram upper bound as being an actual bin: we should assume that there are no values there. So just starting the loop at the last real bin is sufficient. However, I'd prefer to handle the two edge cases (i.e., probe value below histogram lower bound or above histogram upper bound) explicitly, because that makes it more apparent what's going on. There are a couple of other ways in which this code seems insufficiently paranoid to me: * It seems worth spending a couple lines of code at the beginning to verify that the histogram has at least one bin, as it already did for the range-length histogram. Otherwise we've got various divide-by-zero hazards here, along with a need for extra tests in the looping functions. * I think we'd better guard against NaNs coming back from the range subdiff function, as well as the possibility of getting a NaN from the division in get_position (compare [1]). * I am not quite totally sure about this part, but it seems to me that calc_hist_selectivity_contains probably ought to handle the range-bound cases the same as calc_hist_selectivity_contained, even though only the latter is trying to access an unsafe array index. That all leads me to the attached draft patch. It lacks a test case --- I wonder if we can find one that crashes more portably than Michael's? And more eyeballs on calc_hist_selectivity_contains would be good too. regards, tom lane [1] https://www.postgresql.org/message-id/12957.1577974305@sss.pgh.pa.us
Commits
-
Fix edge-case crashes and misestimation in range containment selectivity.
- 8c8b456b5116 10.12 landed
- 784c58da1957 9.5.21 landed
- 70c17a81278c 12.2 landed
- 6bd567b65858 9.4.26 landed
- 5832be6ca4d7 11.7 landed
- 2dd10477cbfc 9.6.17 landed
- 652686a334b4 13.0 landed
-
Use stdbool.h if suitable
- 9a95a77d9d5d 11.0 cited
-
Redesign get_attstatsslot()/free_attstatsslot() for more safety and speed.
- 9aab83fc5039 10.0 cited