From: Wong, Yi Wen <yiwong@amazon.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Wood, Dan <hexpert@amazon.com>
Cc: pgsql-bugs@postgresql.org <pgsql-bugs@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: Old row version in hot chain become visible after a freeze
Date: Tue, 12 Sep 2017 23:29:25 +0000
Message-ID: <1505258965245.51728@amazon.com> (raw)
In-Reply-To: <1505249698051.58904@amazon.com>
References: <CAB7nPqTc0-imDj-5URYW9a5A+yijFv_UUXucDE=3TLyUmZg3AA@mail.gmail.com>
<20170912082203.gq5hutuveqlxpvuc@alvherre.pgsql>
<1505241452245.71279@amazon.com>
<1505249698051.58904@amazon.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-bugs>
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-bugsAttachments:
[application/octet-stream] update_freeze_v4.patch (5.4K, ../1505258965245.51728@amazon.com/2-update_freeze_v4.patch)
download | inline diff:diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex d20f038..65b3bad 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -6405,14 +6405,23 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
Assert(TransactionIdIsValid(xid));
/*
- * If the xid is older than the cutoff, it has to have aborted,- * otherwise the tuple would have gotten pruned away.+ * The updating transaction cannot possibly be still running, but+ * verify whether it has committed, and request to set the+ * COMMITTED flag if so. (We normally don't see any tuples in+ * this state, because they are removed by page pruning before we+ * try to freeze the page; but this can happen if the updating+ * transaction commits after the page is pruned but before+ * HeapTupleSatisfiesVacuum).
*/
if (TransactionIdPrecedes(xid, cutoff_xid))
{
- Assert(!TransactionIdDidCommit(xid));- *flags |= FRM_INVALIDATE_XMAX;- xid = InvalidTransactionId; /* not strictly necessary */+ if (TransactionIdDidCommit(xid))+ *flags = FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;+ else+ {+ *flags |= FRM_INVALIDATE_XMAX;+ xid = InvalidTransactionId; /* not strictly necessary */+ }
}
else
{
@@ -6485,13 +6494,16 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
/*
* It's an update; should we keep it? If the transaction is known
* aborted or crashed then it's okay to ignore it, otherwise not.
- * Note that an updater older than cutoff_xid cannot possibly be- * committed, because HeapTupleSatisfiesVacuum would have returned- * HEAPTUPLE_DEAD and we would not be trying to freeze the tuple.
*
* As with all tuple visibility routines, it's critical to test
* TransactionIdIsInProgress before TransactionIdDidCommit,
* because of race conditions explained in detail in tqual.c.
+ *+ * We normally don't see committed updating transactions earlier+ * than the cutoff xid, because they are removed by page pruning+ * before we try to freeze the page; but it can happen if the+ * updating transaction commits after the page is pruned but+ * before HeapTupleSatisfiesVacuum.
*/
if (TransactionIdIsCurrentTransactionId(xid) ||
TransactionIdIsInProgress(xid))
@@ -6517,13 +6529,6 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
*/
/*
- * Since the tuple wasn't marked HEAPTUPLE_DEAD by vacuum, the- * update Xid cannot possibly be older than the xid cutoff.- */- Assert(!TransactionIdIsValid(update_xid) ||- !TransactionIdPrecedes(update_xid, cutoff_xid));-- /*
* If we determined that it's an Xid corresponding to an update
* that must be retained, additionally add it to the list of
* members of the new Multi, in case we end up using that. (We
@@ -6601,7 +6606,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
*
* It is assumed that the caller has checked the tuple with
* HeapTupleSatisfiesVacuum() and determined that it is not HEAPTUPLE_DEAD
- * (else we should be removing the tuple, not freezing it).+ * (else we should be removing the tuple, not freezing it). The exception to+ * this rule is HEAPTUPLE_DEAD HOT tuplesi; those are treated as RECENTLY_DEAD.
*
* NB: cutoff_xid *must* be <= the current global xmin, to ensure that any
* XID older than it could neither be running nor seen as running by any
@@ -6712,7 +6718,17 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
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;
}
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.cindex 45b1859..97064fe 100644--- a/src/backend/commands/vacuumlazy.c+++ b/src/backend/commands/vacuumlazy.c@@ -2018,17 +2018,21 @@ lazy_record_dead_tuple(LVRelStats *vacrelstats,
ItemPointer itemptr)
{
/*
- * The array shouldn't overflow under normal behavior, but perhaps it- * could if we are given a really small maintenance_work_mem. In that- * case, just forget the last few tuples (we'll get 'em next time).+ * The array must never overflow, since we rely on all deletable tuples+ * being removed; inability to remove a tuple might cause an old XID to+ * persist beyond the freeze limit, which could be disastrous later on.
*/
- if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)- {- vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;- vacrelstats->num_dead_tuples++;- pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,- vacrelstats->num_dead_tuples);- }+ 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")));++ vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] = *itemptr;+ vacrelstats->num_dead_tuples++;+ pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,+ vacrelstats->num_dead_tuples);
}
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: yiwong@amazon.com, alvherre@alvh.no-ip.org, michael.paquier@gmail.com, pg@bowt.ie, hexpert@amazon.com, andres@anarazel.de
Subject: Re: Old row version in hot chain become visible after a freeze
In-Reply-To: <1505258965245.51728@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox