Re: Old row version in hot chain become visible after a freeze

I've spent some time reviewing the new patch and have made a couple of minor changes.

I've made some changes to this chunk:

else if (TransactionIdIsNormal(xid))
{
if (TransactionIdPrecedes(xid, cutoff_xid))
- freeze_xmax = true;
+ {
+ if (HeapTupleHeaderIsHeapOnly(tuple) ||
+ HeapTupleHeaderIsHotUpdated(tuple))
+ {
+ frz->t_infomask |= HEAP_XMAX_COMMITTED;
+ totally_frozen = false;
+ changed = true;
+ }
+ else
+ freeze_xmax = true;
+ }
else
totally_frozen = false;
}

The changes are:

1) To add the line changed = true;
This is necessary to actually set the bits of to HEAP_XMAX_COMMITTED. In repro.sql, this is set anyway
because the xmin is frozen. However, consider a scenario where we have xmin frozen and xmax still live in a
previous VACUUM FREEZE pass, if we do a second pass where HEAP_XMAX_COMMITTED needs to be set,
we will lose that change.

2) Flip the order of HeapTupleHeaderIsHeapOnly(tuple) and HeapTupleHeaderIsHotUpdated(tuple). This is merely a
microoptimization on a non-performance critical path because HeapOnly is a broader condititon than HotUpdated :-)

I've also changed the elog ERROR. This seems necessary for a user to know, because it does mean VACUUM is broken on their
system:

+ if (vacrelstats->num_dead_tuples >= vacrelstats->max_dead_tuples)
+ ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_RESOURCES),
+ errmsg ("dead tuple array overflow: %d dead, %d max",
+ vacrelstats->num_dead_tuples,
+ vacrelstats->max_dead_tuples),
+ errhint("Increase maintenance_work_mem")));

I've also made some changes to comments in the code.

Thanks,
Yi Wen

________________________________________
From: pgsql-bugs-owner(at)postgresql(dot)org <pgsql-bugs-owner(at)postgresql(dot)org> on behalf of Wong, Yi Wen <yiwong(at)amazon(dot)com>
Sent: Tuesday, September 12, 2017 1:54 PM
To: Alvaro Herrera; Michael Paquier
Cc: Peter Geoghegan; Wood, Dan; pgsql-bugs(at)postgresql(dot)org; Andres Freund
Subject: Re: [BUGS] Old row version in hot chain become visible after a freeze

> > > I also tweaked lazy_record_dead_tuple to fail with ERROR if the tuple
> > > cannot be recorded, as observed by Yi Wen. AFAICS that's not reachable
> > > because of the way the array is allocated, so an elog(ERROR) is
> > > sufficient.
>
> I agree the fail is rare (and probably doesn't happen in real cases, although the comment
> does imply with a sufficiently low working_memory it might?). However, I'd dispute that ERROR
> is sufficient --PANIC is probably appropriate here because FREEZE is not WAL-logged until
> the end of the page; so we'd end up with unfrozen Xids hanging around with an appropriately
> timed crash. I wouldn't worry much about the PANIC tripping because like you said,
> this seems unreachable normally.
>
> The errmsg should come with a errhint saying "Increase maintenance_work_mem".

On closer inspection, I misread the code. The freezes are collected instead of executed, so a ERROR would
suffice. We don't need to change it to PANIC. errhint comment remains.

Yi Wen

--
Sent via pgsql-bugs mailing list (pgsql-bugs(at)postgresql(dot)org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs