On 03/04/2018 09:46 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>> On 03/04/2018 08:37 PM, Tom Lane wrote:
>>> Oh, well, that was another problem I had with it: those tests do basically
>>> nothing to ensure that we won't add another such problem in the future.
>
>> I don't follow. How would adding new custom types break the checks? If
>> someone adds a new type along with operators for comparing it with the
>> built-in types (supported by convert_to_scalar), then surely it would
>> hit a code path tested by those tests.
>
> Well, I think the existing bytea bug is a counterexample to that. If
> someone were to repeat that mistake with, say, UUID, these tests would not
> catch it, because none of them would exercise UUID-vs-something-else.
> For that matter, your statement is false on its face, because even if
> somebody tried to add say uuid-versus-int8, these tests would not catch
> lack of support for that in convert_to_scalar unless the specific case of
> uuid-versus-int8 were added to the tests.
>
I suspect we're simply having different expectations what the tests
could/should protect against - my intention was to make sure someone
does not break convert_to_scalar for the currently handled types.
So for example if someone adds the uuid-versus-int8 operator you
mention, then
int8_var > '55e65ca2-4136-4a4b-ba78-cd3fe4678203'::uuid
simply returns false because convert_to_scalar does not handle UUIDs yet
(and you're right none of the tests enforces it), while
uuid_var > 123456::int8
is handled by the NUMERICOID case, and convert_numeric_to_scalar returns
failure=true. And this is already checked by one of the tests.
If someone adds UUID handling into convert_to_scalar, that would need to
include a bunch of new tests, of course.
One reason why I wanted to include the tests was that we've been talking
about reworking this code to allow custom conversion routines etc. In
which case being able to verify the behavior stays the same is quite
valuable, I think.
>> So perhaps the best thing we can do is documenting this in the comment
>> before convert_to_scalar?
>
> I already updated the comment inside it ...
>
Oh, right. Sorry, I missed that somehow.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services