Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold <

On Sat, Jul 2, 2016 at 1:29 PM, Noah Misch <noah@leadboat.com> wrote:
> On Fri, Jul 01, 2016 at 09:00:45AM -0500, Kevin Grittner wrote:

>> I have been looking at several possible fixes, and weighing the
>> pros and cons of each.  I expect to post a patch later today.
>
> This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> http://www.postgresql.org/message-id/20160527025039.GA447393@tornado.leadboat.com

Attached is a patch which fixes this issue, which I will push
Monday unless there are objections.

The problem to be fixed is this:

TOAST values can be pruned or vacuumed away while the heap still
has references to them, but the visibility data is such that you
should not be able to see the referencing heap tuple once the TOAST
value is old enough to clean up.  When the old_snapshot_threshold
is set to allow early pruning, early cleanup of the TOAST values
could occur while a connection can still see the heap row, and the
"snapshot too old" error might not be triggered.  (In practice,
it's fairly hard to hit that, but a test case will be included in a
bit.)  It would even be possible to have an overlapping transaction
which is old enough to create a new value with the old OID after it
is removed, which might even be of a different data type.  The
gymnastics required to hit that are too daunting to have created a
test case, but it seems possible.

The possible fixes considered were these:

(1)  Always vacuum the heap before its related TOAST table.
(2)  Same as (1) but only when old_snapshot_threshold >= 0.
(3)  Allow the special snapshot used for TOAST access to generate
the "snapshot too old" error, so that the modified page from the
pruning/vacuuming (along with other conditions) would cause that
rather than something suggesting corrupt internal structure.
(4)  When looking to read a toasted value for a tuple past the
early pruning horizon, if the value was not found consider it a
"snapshot too old" error.
(5)  Don't vacuum or prune a TOAST table except as part of the heap
vacuum when early pruning is enabled.
(6)  Don't allow early vacuuming/pruning of TOAST values except as
part of the vacuuming of the related heap.

It became evident pretty quickly that the HOT pruning of TOAST
values should not do early cleanup, based on practical concerns of
coordinating that with the heap cleanup for any of the above
options.  What's more, since we don't allow updating of tuples
holding TOAST values, HOT pruning seems to be of dubious value on a
TOAST table in general -- but removing that would be the matter for
a separate patch.  Anyway, this patch includes a small hunk of code
(two lines) to avoid early HOT pruning for TOAST tables.

For the vacuuming, option (6) seems a clear winner, and that is
what this patch implements.  A TOAST table can still be vacuumed on
its own, but in that case it will not use old_snapshot_threshold to
try to do any early cleanup.  We were already normally vacuuming
the TOAST table whenever we vacuumed the related heap; in such a
case it uses the "oldestXmin" used for the heap to vacuum the TOAST
table.  The other options either could not limit errors to cases
when they were really needed or had to pass through way too much
information through many layers to know what actions to take when.

Option (6) basically conditions the call to try to use a more
aggressive cleanup threshold on whether the relation is a TOAST
relation and a flag indicating whether we are in a particular
vacuum function based on the recursive call made from heap vacuum
to cover its TOAST table.  Not the most elegant code, but fairly
straightforward.

The net result is that, like existing production versions, we can
have heap rows pointing to missing TOAST values, but only when
those heap rows are not visible to anyone.

Test case (adapted from one provided by Andres Freund):

-- START WITH:
--   autovacuum = off
--   old_snapshot_threshold = 1

-- connection 1
SHOW autovacuum;
SHOW old_snapshot_threshold;
DROP TABLE IF EXISTS toasted;
CREATE TABLE toasted(largecol text);
INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM
generate_series(1, 10000000);
BEGIN;
DELETE FROM toasted;

-- connection 2
BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
SELECT hashtext(largecol), length(largecol) FROM toasted;

-- connection 1
COMMIT;

-- connection 2
SELECT hashtext(largecol), length(largecol) FROM toasted;

-- connection 1
SELECT hashtext(largecol), length(largecol) FROM toasted;

-- connection 1
/* wait for snapshot threshold to be passed */
SELECT oid FROM pg_class WHERE relname = 'toasted';
VACUUM VERBOSE pg_toast.pg_toast_?;
SELECT hashtext(largecol), length(largecol) FROM toasted;

-- connection 2
SELECT hashtext(largecol), length(largecol) FROM toasted;

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company