Date: Mon, 11 Sep 2017 16:01:03 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Peter Geoghegan <pg@bowt.ie>, "Wood, Dan" <hexpert@amazon.com>,
	"pgsql-bugs@postgresql.org" <pgsql-bugs@postgresql.org>,
	Andres Freund <andres@anarazel.de>,
	"Wong, Yi Wen" <yiwong@amazon.com>
Subject: Re: Old row version in hot chain become visible after a freeze
Message-ID: <20170911140103.5akxptyrwgpc25bw@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="hmasspo3mbwypkk2"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAB7nPqSRbohi9L0EH4QMZMPYT_mZ3YQg0xf5QUxvdSmixd7-tQ@mail.gmail.com>
User-Agent: NeoMutt/20170306-137-4415bd-dirty (1.8.0)
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org


--hmasspo3mbwypkk2
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Michael Paquier wrote:
> On Wed, Sep 6, 2017 at 7:40 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Otherwise, we risk marking the page as all-frozen, and it would be
> > skipped by vacuum.  If we never come around to HOT-pruning the page, a
> > non-permanent xid (or a multixact? not sure that that can happen;
> > probably not) would linger unnoticed and cause a DoS condition later
> > ("cannot open file pg_clog/1234") when the tuple header is read.
> 
> Yes, you are definitely right here.
> 
> On Wed, Sep 6, 2017 at 10:27 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Another thing is that if you're going through heap_copy_data, the tuple
> > is only checked for DEAD, not RECENTLY_DEAD.  Maybe there is no bug here
> > but I'm not sure yet.
> 
> Not sure to what you are referring here. Perhaps heap_prune_chain()?
> It seems to me that it does the right thing.

I meant copy_heap_data, which also "freezes" tuples while copying the
heap.  In the end, I concluded the fact that it runs with access
exclusive lock is sufficient protection.  I was worried that in the
future we might improve that code and enable it to run without AEL, but
after looking at it a little more, it sounds like a big enough project
that this is going to be the least of its troubles.

Anyway, in order to test this, I tried running this one-liner (files
attached):
  psql -f /tmp/repro.sql ; for i in `seq 1 50`; do psql --no-psqlrc -f /tmp/lock.sql & done

(I also threw in a small sleep between heap_page_prune and
HeapTupleSatisfiesVacuum while testing, just to widen the problem window
to hopefully make any remaining problems more evident.)

This turned up a few different failure modes, which I fixed until no
further problems arose.  With the attached patch, I no longer see any
failures (assertion failures) or misbehavior (additional rows), in a few
dozen runs, which were easy to come by with the original code.  The
resulting patch, which I like better than the previously proposed idea
of skipping the freeze, takes the approach of handling freeze correctly
for the cases where the tuple still exists after pruning.

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 regret my inability to turn the oneliner into a committable test case,
but I think that's beyond what I can do for now.


FWIW running this against 9.2 I couldn't detect any problems, which I
suppose was expected.  Changing the lock mode in the test file to FOR
SHARE I only see an enormous amount of deadlocks reported (which makes
me confident that doing multixact stuff in 9.3 was a good thing, despite
what bug chasers may think.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--hmasspo3mbwypkk2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="repro.sql"

drop table t;
create table t (id int primary key, name char(3), x integer);

insert into t values (1, '111', 0);
insert into t values (3, '333', 0);

--hmasspo3mbwypkk2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="lock.sql"

begin;

values ('for share'), ('') order by random() limit 1 \gset

select id from t where id=3 :column1 ;
select pg_sleep(random());
update t set x=x+1 where id=3;

select random() > .5 as commit \gset

\if :commit
   commit;
\else
   rollback;
\endif

vacuum freeze t;
select ctid, xmin, xmax, id, x from t;

--hmasspo3mbwypkk2
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="update_freeze_v3.patch"
Content-Transfer-Encoding: quoted-printable

commit 75b33a84b9b4ce83aac29c4454837a0aef71bfe8=1B[m
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Mon Sep 11 15:15:24 2017 +0200
CommitDate: Mon Sep 11 15:17:02 2017 +0200

    My proposed fix

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/hea=
pam.c
index e61dd771d0..84357e4954 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -6388,14 +6388,23 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infom=
ask,
 			Assert(TransactionIdIsValid(xid));
=20
 			/*
-			 * 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 |=3D FRM_INVALIDATE_XMAX;
-				xid =3D InvalidTransactionId; /* not strictly necessary */
+				if (TransactionIdDidCommit(xid))
+					*flags =3D FRM_MARK_COMMITTED | FRM_RETURN_IS_XID;
+				else
+				{
+					*flags |=3D FRM_INVALIDATE_XMAX;
+					xid =3D InvalidTransactionId; /* not strictly necessary */
+				}
 			}
 			else
 			{
@@ -6475,6 +6484,12 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infoma=
sk,
 			 * 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))
@@ -6500,13 +6515,6 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infoma=
sk,
 			 */
=20
 			/*
-			 * 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
@@ -6695,7 +6703,16 @@ heap_prepare_freeze_tuple(HeapTupleHeader tuple, Tra=
nsactionId cutoff_xid,
 	else if (TransactionIdIsNormal(xid))
 	{
 		if (TransactionIdPrecedes(xid, cutoff_xid))
-			freeze_xmax =3D true;
+		{
+			if (HeapTupleHeaderIsHotUpdated(tuple) ||
+				HeapTupleHeaderIsHeapOnly(tuple))
+			{
+				frz->t_infomask |=3D HEAP_XMAX_COMMITTED;
+				totally_frozen =3D false;
+			}
+			else
+				freeze_xmax =3D true;
+		}
 		else
 			totally_frozen =3D false;
 	}
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuu=
mlazy.c
index 45b1859475..30b1c08c6c 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -2018,17 +2018,17 @@ 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] =3D *itemptr;
-		vacrelstats->num_dead_tuples++;
-		pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
-									 vacrelstats->num_dead_tuples);
-	}
+	if (vacrelstats->num_dead_tuples >=3D vacrelstats->max_dead_tuples)
+		elog(ERROR, "dead tuple array overflow");
+
+	vacrelstats->dead_tuples[vacrelstats->num_dead_tuples] =3D *itemptr;
+	vacrelstats->num_dead_tuples++;
+	pgstat_progress_update_param(PROGRESS_VACUUM_NUM_DEAD_TUPLES,
+								 vacrelstats->num_dead_tuples);
 }
=20
 /*

--hmasspo3mbwypkk2
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


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

--hmasspo3mbwypkk2--