Thread: contrib/pg_visibility craps out in assert-enabled builds

So I tried using pg_visibility's pg_check_visible() as part of
testing the business with pg_upgrade generating faulty visibility
maps on bigendian servers, and it instantly generated an assert
failure here:

#2  0x0041de78 in ExceptionalCondition (conditionName=<value temporarily unavailable, due to optimizations>,
errorType=<valuetemporarily unavailable, due to optimizations>, fileName=<value temporarily unavailable, due to
optimizations>,lineNumber=<value temporarily unavailable, due to optimizations>) at assert.c:54
 
#3  0x0045c410 in HeapTupleSatisfiesVacuum (htup=0x0, OldestXmin=9170, buffer=2958) at tqual.c:1169
#4  0x00a41c3c in tuple_all_visible (tup=0xbfffd8e4, OldestXmin=9170, buffer=<value temporarily unavailable, due to
optimizations>)at pg_visibility.c:719
 
#5  0x00a420a8 in collect_corrupt_items (relid=46802, all_visible=<value temporarily unavailable, due to
optimizations>,all_frozen=<value temporarily unavailable, due to optimizations>) at pg_visibility.c:630
 
#6  0x00a4262c in pg_check_visible (fcinfo=0x104b704) at pg_visibility.c:328

The problem seems to be that HeapTupleSatisfiesVacuum asserts
Assert(ItemPointerIsValid(&htup->t_self));

while collect_corrupt_items hasn't bothered to set up the t_self
field of the HeapTupleData it's passing in.  This would imply that
you never tested this code in an assert-enabled build, which I find
surprising.  Am I missing something?

(I'm not really sure *why* HeapTupleSatisfiesVacuum contains this
Assert, because it doesn't do anything with t_self, but nonetheless
the Assert has been there for several years.  Seems to have been
inserted by you, in fact.)
        regards, tom lane

On Fri, Sep 30, 2016 at 10:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I tried using pg_visibility's pg_check_visible() as part of
> testing the business with pg_upgrade generating faulty visibility
> maps on bigendian servers, and it instantly generated an assert
> failure here:
>
> #2  0x0041de78 in ExceptionalCondition (conditionName=<value temporarily unavailable, due to optimizations>,
errorType=<valuetemporarily unavailable, due to optimizations>, fileName=<value temporarily unavailable, due to
optimizations>,lineNumber=<value temporarily unavailable, due to optimizations>) at assert.c:54 
> #3  0x0045c410 in HeapTupleSatisfiesVacuum (htup=0x0, OldestXmin=9170, buffer=2958) at tqual.c:1169
> #4  0x00a41c3c in tuple_all_visible (tup=0xbfffd8e4, OldestXmin=9170, buffer=<value temporarily unavailable, due to
optimizations>)at pg_visibility.c:719 
> #5  0x00a420a8 in collect_corrupt_items (relid=46802, all_visible=<value temporarily unavailable, due to
optimizations>,all_frozen=<value temporarily unavailable, due to optimizations>) at pg_visibility.c:630 
> #6  0x00a4262c in pg_check_visible (fcinfo=0x104b704) at pg_visibility.c:328
>
> The problem seems to be that HeapTupleSatisfiesVacuum asserts
>
>         Assert(ItemPointerIsValid(&htup->t_self));
>
> while collect_corrupt_items hasn't bothered to set up the t_self
> field of the HeapTupleData it's passing in.  This would imply that
> you never tested this code in an assert-enabled build, which I find
> surprising.  Am I missing something?

My standard build script uses --enable-cassert, so I doubt that's the
case.  It's more likely that on my system it just happened to find
some non-zero garbage at that point on the stack that made it not fail
the assertion.

/me tests.

Yep.  I just checked out 71d05a2c7b82379bb1013a0e338906349c54ed85 and
added an elog() immediately after the call to
HeapTupleSatisfiesVacuum(), and I can hit that elog() without failing
an assertion.  Furthermore I can see that debug_assertions = on.

> (I'm not really sure *why* HeapTupleSatisfiesVacuum contains this
> Assert, because it doesn't do anything with t_self, but nonetheless
> the Assert has been there for several years.  Seems to have been
> inserted by you, in fact.)

I suspect if we reviewed the discussion leading up to
0518eceec3a1cc2b71da04e839f05f555fdd8567 we'd find it.  I don't
actually recall the discussion of checking t_self specifically, but I
know checking t_tableOid had been requested multiple times.  I suspect
we just decided to check both.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Sep 30, 2016 at 10:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The problem seems to be that HeapTupleSatisfiesVacuum asserts
>> Assert(ItemPointerIsValid(&htup->t_self));
>> while collect_corrupt_items hasn't bothered to set up the t_self
>> field of the HeapTupleData it's passing in.  This would imply that
>> you never tested this code in an assert-enabled build, which I find
>> surprising.  Am I missing something?

> My standard build script uses --enable-cassert, so I doubt that's the
> case.  It's more likely that on my system it just happened to find
> some non-zero garbage at that point on the stack that made it not fail
> the assertion.

Yeah, after I looked closer at what the Assert is actually testing,
I realized it was just blind luck that I'd managed to see it fail.
It's a pretty weak test :-(.  Anyway, fixed now.
        regards, tom lane