Hi,

I noticed that heap_tuple_needs_freeze might return true in cases where
the Xmax is leftover junk from somebody who set HEAP_XMAX_INVALID in the
far past without resetting the Xmax value itself to Invalid.  I think
this is incorrect usage; the rule, I think, is that one shouldn't even
read Xmax at all unless HEAP_XMAX_INVALID is reset.

This might cause unnecessary acquisitions of the cleanup lock, if a
tuple is deemed freezable when in fact it isn't.

Suggested patch attached.  I'd backpatch this as far as it applies
cleanly.

-- 
Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> ! 	if (!(tuple->t_infomask & HEAP_XMAX_INVALID))
>   	{
> ! 		if (!(tuple->t_infomask & HEAP_XMAX_IS_MULTI))

How about just one test,

	if (!(tuple->t_infomask & (HEAP_XMAX_INVALID | HEAP_XMAX_IS_MULTI)))

But other than that quibble, yeah, it's a bug.  XMAX_INVALID means just
that: the xmax is not to be thought valid.

			regards, tom lane

On Wed, Feb 1, 2012 at 8:01 PM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Suggested patch attached.  I'd backpatch this as far as it applies
> cleanly.

This is new code in 9.2, but it's modelled on heap_freeze_tuple(), which is old.

I'm not convinced that it's a bug.  Suppose that xmax is set but is
hinted as invalid.  We process the table and advanced relfrozenxid;
then, we crash.  After recovery, it's possible that the hint bit is
gone (after all, setting hint bits isn't WAL-logged).  Now we're in
big trouble, because the next CLOG lookup on that xmax value might not
happen until it's been reused, and we might get a different answer
than before.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm not convinced that it's a bug.  Suppose that xmax is set but is
> hinted as invalid.

XMAX_INVALID is not a "hint".  When it's set, the contents of the field
must be presumed to be garbage.  Any code failing to adhere to that rule
is broken.

> We process the table and advanced relfrozenxid;
> then, we crash.  After recovery, it's possible that the hint bit is
> gone (after all, setting hint bits isn't WAL-logged).  Now we're in
> big trouble, because the next CLOG lookup on that xmax value might not
> happen until it's been reused, and we might get a different answer
> than before.

I believe we have adequate defenses against that, and even if we did
not, that doesn't make the code in question less wrong.

			regards, tom lane

On Thu, Feb 2, 2012 at 11:27 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I'm not convinced that it's a bug.  Suppose that xmax is set but is
>> hinted as invalid.
>
> XMAX_INVALID is not a "hint".  When it's set, the contents of the field
> must be presumed to be garbage.  Any code failing to adhere to that rule
> is broken.
>
>> We process the table and advanced relfrozenxid;
>> then, we crash.  After recovery, it's possible that the hint bit is
>> gone (after all, setting hint bits isn't WAL-logged).  Now we're in
>> big trouble, because the next CLOG lookup on that xmax value might not
>> happen until it's been reused, and we might get a different answer
>> than before.
>
> I believe we have adequate defenses against that, and even if we did
> not, that doesn't make the code in question less wrong.

I believe the adequate defense that we have is precisely the logic you
are proposing to change.  Regardless of whether you want to call
XMAX_INVALID a hint or, say, a giant tortoise, I am fairly sure that
we don't WAL-log setting it.  That means that a bit set before a crash
won't necessarily still be set after a crash.  But the corresponding
relfrozenxid advancement will be WAL-logged, leading to the problem
scenario I described.

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

On Thu, Feb 2, 2012 at 12:45 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I believe the adequate defense that we have is precisely the logic you
> are proposing to change.  Regardless of whether you want to call
> XMAX_INVALID a hint or, say, a giant tortoise, I am fairly sure that
> we don't WAL-log setting it.  That means that a bit set before a crash
> won't necessarily still be set after a crash.  But the corresponding
> relfrozenxid advancement will be WAL-logged, leading to the problem
> scenario I described.

To put that another way, the problem isn't that we might have code
somewhere in the system that ignores HEAP_XMAX_INVALID.  The problem
is that HEAP_XMAX_INVALID might not still be set on that tuple the
next time somebody looks at it, if a database crash intervenes after
that bit is set and before it is flushed to disk.

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