HeapTupleData.t_self garbage values

According to htup.h:

* t_self and t_tableOid should be valid if the HeapTupleData points
* to a disk buffer, or if it represents a copy of a tuple on disk.
* They should be explicitly set invalid in manufactured tuples.

In the heap_hot_search_buffer function of heapam.c this is not true.
I can't find a clear explanation of why that is. I'm assuming "it
just doesn't matter" here, but at a minimum it seems worth a
comment. It's not immediately obvious to me what the random garbage
from the stack would be replaced with if we were to try to make the
above comment true.

Quick brain dump on the topic, anyone?

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> According to htup.h:
> * t_self and t_tableOid should be valid if the HeapTupleData points
> * to a disk buffer, or if it represents a copy of a tuple on disk.
> * They should be explicitly set invalid in manufactured tuples.

> In the heap_hot_search_buffer function of heapam.c this is not true.
> I can't find a clear explanation of why that is. I'm assuming "it
> just doesn't matter" here, but at a minimum it seems worth a
> comment. It's not immediately obvious to me what the random garbage
> from the stack would be replaced with if we were to try to make the
> above comment true.

What that comment is recommending is

ItemPointerSetInvalid(&(tuple.t_self));

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> According to htup.h:
>> * t_self and t_tableOid should be valid if the HeapTupleData
>> * points to a disk buffer, or if it represents a copy of a tuple
>> * on disk. They should be explicitly set invalid in manufactured
>> * tuples.
>
>> In the heap_hot_search_buffer function of heapam.c this is not
>> true.

> What that comment is recommending is
>
> ItemPointerSetInvalid(&(tuple.t_self));

Aren't those tuples pointing to a disk buffer? I know how to set an
invalid pointer, but I thought that was only for manufactured
tuples?

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> ItemPointerSetInvalid(&(tuple.t_self));

> Aren't those tuples pointing to a disk buffer?

Oh, I should have looked at the code before commenting ;-).

Yeah, the correct TID value would be ItemPointerGetBlockNumber(tid)
plus the current offnum. However we don't have enough information
in this function to set t_tableOid correctly, so maybe it would be
best to just set both fields invalid. Or do nothing --- AFAICS none
of the uses of the heapTuple look at those fields. Is it worth a few
extra cycles to initialize unused fields of a short-lived heapTuple?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Yeah, the correct TID value would be
> ItemPointerGetBlockNumber(tid) plus the current offnum.

Thanks!

> However we don't have enough information in this function to set
> t_tableOid correctly, so maybe it would be best to just set both
> fields invalid. Or do nothing --- AFAICS none of the uses of the
> heapTuple look at those fields. Is it worth a few extra cycles to
> initialize unused fields of a short-lived heapTuple?

At a minimum, it might be good to qualify the comment in htup.h and
add a comment where there is an exception. This can be startling in
a debugger if you don't know that the comment isn't really true.
(And I've found another place where t_tableOid isn't set, but it is
apparently benign; that's without an exhaustive search.)

I could put forward a comment-only patch per the above if there are
no objections.

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> At a minimum, it might be good to qualify the comment in htup.h and
> add a comment where there is an exception. This can be startling in
> a debugger if you don't know that the comment isn't really true.
> (And I've found another place where t_tableOid isn't set, but it is
> apparently benign; that's without an exhaustive search.)

> I could put forward a comment-only patch per the above if there are
> no objections.

I don't want to put weasel wording into the comment. If you're bothered
by the discrepancy then we should fix the code to initialize all the
struct fields.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I don't want to put weasel wording into the comment. If you're
> bothered by the discrepancy then we should fix the code to
> initialize all the struct fields.

Maybe for 9.1. At this point, unless it's breaking something I
can't see that anything beyond a comment would be justified.

Longer term, it's hard enough getting one's head around a million
lines of code without false statements in the comments.

Thanks again.

-Kevin