On 03/04/2018 05:59 PM, Tom Lane wrote: > Tomas Vondra <tomas.von...@2ndquadrant.com> writes: >> On 03/04/2018 02:37 AM, Tom Lane wrote: >>> I was kind of underwhelmed with these test cases, too, so I didn't >>> commit them. But they were good for proving that the bytea bug >>> wasn't hypothetical :-) > >> Underwhelmed in what sense? Should the tests be constructed in some >> other way, or do you think it's something that doesn't need the tests? > > The tests seemed pretty ugly, and I don't think they were doing much to > improve test coverage by adding all those bogus operators. Now, a look at > https://coverage.postgresql.org/src/backend/utils/adt/selfuncs.c.gcov.html > says that our test coverage for convert_to_scalar stinks, but we could > (and probably should) improve that just by testing extant operators. >
Hmmm, OK. I admit the tests weren't a work of art, but I didn't consider them particularly ugly either. But that's a matter of taste, I guess. Using existing operators was the first thing I considered, of course, but to exercise this part of the code you need an operator that: 1) exercises uses ineq_histogram_selectivity (so either scalarineqsel or prefix_selectivity) 2) has oprright != oprleft 3) either oprright or oprleft is unsupported by convert_to_scalar I don't think we have such operator (built-in or in regression tests). At least I haven't found one, and this was the simplest case that I've been able to come up with. But maybe you had another idea. > A concrete argument for not creating those operators is that they pose a > risk of breaking concurrently-running tests by capturing inexact argument > matches (cf CVE-2018-1058). There are ways to get around that, eg run > the whole test inside a transaction we never commit; but I don't really > think we need the complication. > I don't think the risk of breaking other tests is very high - the operators were sufficiently "bogus" to make it unlikely, I think. But it's simple to mitigate that by either running the test in a transaction, dropping the operators at the end of the test, or just using some sufficiently unique operator name (and not '>'). FWIW I originally constructed the tests merely to verify that the fix actually fixes the issue, but I think we should add some tests to make sure it does not get broken in the future. It took quite a bit of time until someone even hit the issue, so a breakage may easily get unnoticed for a long time. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
