On 1/16/18 01:14, Michael Paquier wrote:
>> I don't see it either. I think the actually useful parts of that patch
>> were to declare record_image_cmp's result correctly as int rather than
>> bool, and to cope with varlena fields of unequal size. Unfortunately
>> there seems to be no contemporaneous mailing list discussion, so
>> it's not clear what Kevin based this change on.
>
> This was a hotfix after a buildfarm breakage, the base commit being
> f566515.
>
>> Want to try reverting the GET_X_BYTES() parts of it and see if the
>> buildfarm complains?
>
> So, Peter, are you planning to do so?
Here is a proposed patch set to clean this up. First, add some test
coverage for record_image_cmp. (There wasn't any, only for
record_image_eq as part of MV testing.) Then, remove the GET_ macros
from record_image_{eq,cmp}. And finally remove all that byte-masking
stuff from postgres.h.
>> Note if that if we simplify the GetDatum macros, it's possible that
>> record_image_cmp would change behavior, since it might now see signed not
>> unsigned values depending on whether the casts do sign extension or not.
>> Not sure if that'd be a problem.
I wasn't able to construct such a case. Everything still works unsigned
end-to-end, I think. But if you can think of a case, we can throw it
into the tests. The tests already contain cases of comparing positive
and negative integers.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services