Message-ID: <4D75EAC4.7090008@enterprisedb.com>
Date: Tue, 08 Mar 2011 10:37:24 +0200
From: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; fi-FI;
	rv:1.9.1.16) Gecko/20101226 Icedove/3.0.11
MIME-Version: 1.0
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
CC: Greg Stark <gsstark@mit.edu>, daveg <daveg@sonic.net>
Subject: Re: Re: PD_ALL_VISIBLE flag was incorrectly set happend
	during repeatable vacuum
References: <AANLkTikw==USr3+4Mt+w97P3ymux-Bhn0G7Ad0ZJHbqG@mail.gmail.com>
	<4D2D6A56.9060808@enterprisedb.com>
	<20110228212832.GH26397@sonic.net>
	<1B3CCBDD-4AC6-404A-9E26-DB4CA2554228@endpoint.com>
	<AANLkTi=xq4SV6jwA7txvrbEWocyeiU6XiTG=-zVy+7Dq@mail.gmail.com>
	<AANLkTi=Ye7AoF7_uVQfPVn-ok2VOp65mi0xU3H7kCke3@mail.gmail.com>
	<20110303071208.GA4053@sonic.net>
	<4D6F4E5D.6080507@enterprisedb.com>
	<20110304090031.GE4053@sonic.net>
	<4D710ABD.5090103@enterprisedb.com>
	<20110307235340.GA21941@sonic.net>
	<AANLkTi=54GF1MJN3FSXL2gr9xG_aB09y_taT8daUwz4Z@mail.gmail.com>
	<4D75E201.20402@enterprisedb.com>
In-Reply-To: <4D75E201.20402@enterprisedb.com>
Content-Type: multipart/mixed; boundary="------------080206030404070600000104"

This is a multi-part message in MIME format.
--------------080206030404070600000104
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 08.03.2011 10:00, Heikki Linnakangas wrote:
> Another idea is to give up on the warning when it appears that
> oldestxmin has moved backwards, and assume that it's actually fine. We
> could still warn in other cases where the flag appears to be incorrectly
> set, like if there is a deleted tuple on the page.

This is probably a better idea at least in back-branches. It also 
handles the case of twiddling vacuum_defer_cleanup_age, which tracking 
two xmins per transactions would not handle.

Here's a patch. I also changed the warning per Robert's suggestion. 
Anyone see a hole in this?

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

--------------080206030404070600000104
Content-Type: text/x-diff;
 name="silence-bogus-all-visible-warning-1.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="silence-bogus-all-visible-warning-1.patch"

diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 67e0be9..dbd2e8d 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -332,6 +332,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 		Size		freespace;
 		bool		all_visible_according_to_vm = false;
 		bool		all_visible;
+		bool		has_dead_tuples;
 
 		/*
 		 * Skip pages that don't require vacuuming according to the visibility
@@ -475,6 +476,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 		 * requiring freezing.
 		 */
 		all_visible = true;
+		has_dead_tuples = false;
 		nfrozen = 0;
 		hastup = false;
 		prev_dead_count = vacrelstats->num_dead_tuples;
@@ -613,6 +615,7 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 			{
 				lazy_record_dead_tuple(vacrelstats, &(tuple.t_self));
 				tups_vacuumed += 1;
+				has_dead_tuples = true;
 			}
 			else
 			{
@@ -671,9 +674,22 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
 			PageSetAllVisible(page);
 			SetBufferCommitInfoNeedsSave(buf);
 		}
-		else if (PageIsAllVisible(page) && !all_visible)
+		/*
+		 * It's possible for the value returned by GetOldestXmin() to move
+		 * backwards, so it's not wrong for us to see tuples that appear to
+		 * not be visible to everyone yet, while PD_ALL_VISIBLE is already
+		 * set. The real safe xmin value never moves backwards, but
+		 * GetOldestXmin() is conservative and sometimes returns a value
+		 * that's unnecessarily small, so if we see that contradiction it
+		 * just means that the tuples that we think are not visible to
+		 * everyone yet actually are, and the PD_ALL_VISIBLE flag is correct.
+		 *
+		 * There shouldn't be any dead tuples on a page with PD_ALL_VISIBLE
+		 * set, however.
+		 */
+		else if (PageIsAllVisible(page) && has_dead_tuples)
 		{
-			elog(WARNING, "PD_ALL_VISIBLE flag was incorrectly set in relation \"%s\" page %u",
+			elog(WARNING, "page containing dead tuples is marked as all-visible in relation \"%s\" page %u",
 				 relname, blkno);
 			PageClearAllVisible(page);
 			SetBufferCommitInfoNeedsSave(buf);

--------------080206030404070600000104--