Re: pgsql: Fix blatantly broken record_image_cmp() logic for pass-by-value

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Doesn't anybody here pay attention to compiler warnings?

>> http://git.postgresql.org/pg/commitdiff/28858811472f316f73eba0e564837088fc8c6ccd

> I don't get a warning on this with either of these compilers,
> either with or without asserts enabled:

Perhaps you built with -O0? At least in older versions of gcc, you need
at least -O1 to get uninitialized-variable warnings. (I've heard some
claims that the latest versions of gcc don't require that anymore.)
I don't recommend -O0 as your default optimization level, precisely
because of this.

> I really don't like the above "fix", since it only
> suppresses the warning without fixing the fundamental problem --
> which is that if there is a pass-by-value type with a disallowed
> length the comparison would not generate an error in a no-assert
> build. The above patch only changes things from an unpredictable
> wrong behavior to a predictable wrong behavior in such cases.

Uh, no, the code was flat out wrong regardless of the possibility that
we reach and fall through the Assert. As an example, in the path for
4-byte pass-by-val:

if (GET_4_BYTES(values1[i1]) !=
GET_4_BYTES(values2[i2]))
{
cmpresult = (GET_4_BYTES(values1[i1]) <
GET_4_BYTES(values2[i2])) ? -1 : 1;
}

if the two values are in fact equal then this falls through leaving
cmpresult uninitialized, rather than setting it to zero as it should be;
which is the case my patch was meant to correct. This perhaps escaped
testing because we aren't using record_image_cmp in a way that exposes
false nonequality results.

I'm not particularly excited about the possibility that attlen might
not be either 1,2,4, or 8; I'm pretty sure there are lots of other
places that would go belly-up with such a problem. However, if that
bothers you the fix is to replace the Assert with an elog(ERROR),
not to remove the initialization.

> I think something like the attached would make more sense.

That patch reintroduces the bug.

regards, tom lane