Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: new heapcheck contrib module
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: 
 <CA+TgmoaX7sPUKyHf+1bec_OJgDhvPBw_9Jt9XbEAPxhcF1YVVw@mail.gmail.com>
Date: Thu, 30 Jul 2020 13:18:01 -0700
Cc: Dilip Kumar <dilipbalaut@gmail.com>,
 Peter Geoghegan <pg@bowt.ie>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <DCD52DE8-C9EF-4C10-BFCC-5293E4742C30@enterprisedb.com>
References: <12ED3DA8-25F0-4B68-937D-D907CFBF08E7@enterprisedb.com>
 <20200420194210.sevnpi4lyghoanxi@alap3.anarazel.de>
 <23E60C3C-C20A-4508-84AC-E0CEA59E784A@enterprisedb.com>
 <CA+TgmobAm8vB1MNQ1taO1et+HxTN8ULnQvEG+grNDqNQTBjbtA@mail.gmail.com>
 <F845AC50-A479-43F3-ADFB-861A4D2AD287@enterprisedb.com>
 <5F743835-3399-419C-8324-2D424237E999@enterprisedb.com>
 <CAFiTN-syyHc3jZoou51v0SR8z0POoNfktqEO6MaGig4YS8mosA@mail.gmail.com>
 <E6F4258A-B661-4F2C-8A35-278C9436930A@enterprisedb.com>
 <CAFiTN-s-VX3DLfqv7OOo0zGCTOZ3xVU9UZ6GgzBZdJTPNnYFNg@mail.gmail.com>
 <885D2EC9-7D06-4903-A17D-72C184A1E1C5@enterprisedb.com>
 <CAFiTN-uYg_PiZ854=5R-D9skratapAHKhaDAG7fhJv-Vn5UhaQ@mail.gmail.com>
 <5CC85F28-F210-47AC-8DA8-EC4E82AF3FD5@enterprisedb.com>
 <CAFiTN-sP5iFJGERTq4a4OLTCzjZKhLC5P6_k2bxQKXnuvdmzGA@mail.gmail.com>
 <CAFiTN-tTg0R5AHvXC1ro9XphroX1wUtc5t91hU_+7DeGTpE5VQ@mail.gmail.com>
 <0C0349EB-DD85-4DA8-82C7-DF70AB714592@enterprisedb.com>
 <CAFiTN-uD3WSeggU20yU6voGou6BDb-cQcfkE8B1rAzWKZVK-Bw@mail.gmail.com>
 <BA6E870F-241C-4DDB-B92D-DB39E679C7D4@enterprisedb.com>
 <CA+TgmobQUdfM2Q7WAR606RR2YehCZ5GHRKBc-VWfRj-1ZUKqDA@mail.gmail.com>
 <C4A3E5AE-CBB2-4E3F-8BAE-29EE216762C2@enterprisedb.com>
 <CA+TgmoaX7sPUKyHf+1bec_OJgDhvPBw_9Jt9XbEAPxhcF1YVVw@mail.gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Precedence: bulk


> On Jul 29, 2020, at 12:52 PM, Robert Haas <robertmhaas@gmail.com> =
wrote:
>=20
> On Mon, Jul 20, 2020 at 5:02 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>> I've made the options 'all-visible', 'all-frozen', and 'none'.  It =
defaults to 'none'.
>=20
> That looks nice.
>=20
>>> I guess that
>>> could still be expensive if there's a lot of them, but needing
>>> ShareUpdateExclusiveLock rather than only AccessShareLock is a =
little
>>> unfortunate.
>>=20
>> I welcome strategies that would allow for taking a lesser lock.
>=20
> I guess I'm not seeing why you need any particular strategy here. Say
> that at the beginning you note the starting relfrozenxid of the table
> -- I think I would lean toward just ignoring datfrozenxid and the
> cluster-wide value completely. You also note the current value of the
> transaction ID counter. Those are the two ends of the acceptable
> range.
>=20
> Let's first consider the oldest acceptable XID, bounded by
> relfrozenxid. If you see a value that is older than the relfrozenxid
> value that you noted at the start, it is definitely invalid. If you
> see a newer value, it could still be older than the table's current
> relfrozenxid, but that doesn't seem very worrisome. If the user
> vacuumed the table while they were running this tool, they can always
> run the tool again afterward if they wish. Forcing the vacuum to wait
> by taking ShareUpdateExclusiveLock doesn't actually solve anything
> anyway: you STILL won't notice any problems the vacuum introduces, and
> in fact you are now GUARANTEED not to notice them, plus now the vacuum
> happens later.
>=20
> Now let's consider the newest acceptable XID, bounded by the value of
> the transaction ID counter. Any time you see a newer XID than the last
> value of the transaction ID counter that you observed, you go observe
> it again. If the value from the table still looks invalid, then you
> complain about it. Either way, you remember the new observation and
> check future tuples against that value. I think the patch is already
> doing this anyway; if it weren't, you'd need an even stronger lock,
> one sufficient to prevent any insert/update/delete activity on the
> table altogether.
>=20
> Maybe I'm just being dense here -- exactly what problem are you =
worried about?

Per tuple, tuple_is_visible() potentially checks whether the xmin or =
xmax committed via TransactionIdDidCommit.  I am worried about =
concurrent truncation of clog entries causing I/O errors on SLRU lookup =
when performing that check.  The three strategies I had for dealing with =
that were taking the XactTruncationLock (formerly known as =
CLogTruncationLock, for those reading this thread from the beginning), =
locking out vacuum, and the idea upthread from Andres about setting =
PROC_IN_VACUUM and such.  Maybe I'm being dense and don't need to worry =
about this.  But I haven't convinced myself of that, yet.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company