Re: [BUGS] 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@postgresql.org <pgsql-bugs-owner@postgresql.org> on behalf of Wong, Yi Wen <yiwong@amazon.com>
Sent: Tuesday, September 12, 2017 1:54 PM
To: Alvaro Herrera; Michael Paquier
Cc: Peter Geoghegan; Wood, Dan; pgsql-bugs@postgresql.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

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