On Wed, Jul 13, 2016 at 4:14 PM, Andres Freund <and...@anarazel.de> wrote:

> Currently, if old_snapshot_threshold is enabled, vacuum is prevented
> from truncating tables:
> static bool
> should_attempt_truncation(LVRelStats *vacrelstats)
> {
>         BlockNumber possibly_freeable;
>         possibly_freeable = vacrelstats->rel_pages - 
> vacrelstats->nonempty_pages;
>         if (possibly_freeable > 0 &&
>                 (possibly_freeable >= REL_TRUNCATE_MINIMUM ||
>           possibly_freeable >= vacrelstats->rel_pages / 
>                 old_snapshot_threshold < 0)
>                 return true;
>         else
>                 return false;
> }
> (note the old_snapshot_threshold < 0 condition).
> That appears to not be mentioned in a comment, the commit message or the
> the docs. I think this definitely needs to be prominently documented.
> FWIW, afaics that's required because new pages don't have an LSN, so we
> can't necessarily detect that a truncated and re-extended relation,
> wouldn't be valid. Although I do wonder if there isn't a less invasive
> way to do that.

There would be no problem on re-extended pages because they would
have a new enough LSN to cause a "snapshot too old" error; it is
when they are missing that the problem exists.  The possible
alternative that looked best to me was to leave a "guard page" to
cover sequential scans, with LSN set to (at least) the latest of
itself or any truncated page.  I think WAL would need to be
generated to cover the LSN update.  If you combined that with
treating an index leaf tuple pointing to a missing page as
"snapshot too old" I think things would work, but it's not clear to
me that it's worth the complexity.

Attached pushed.

Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 4e8c982..c9e0ec2 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2092,6 +2092,15 @@ include_dir 'conf.d'
+         When this feature is enabled, freed space at the end of a relation
+         cannot be released to the operating system, since that could remove
+         information needed to detect the <literal>snapshot too old</>
+         condition.  All space allocated to a relation remains associated with
+         that relation for reuse only within that relation unless explicitly
+         freed (for example, with <command>VACUUM FULL</>).
+        </para>
+        <para>
          This setting does not attempt to guarantee that an error will be
          generated under any particular circumstances.  In fact, if the
          correct results can be generated from (for example) a cursor which
diff --git a/src/backend/commands/vacuumlazy.c 
index 4075f4d..231e92d 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1663,6 +1663,15 @@ lazy_cleanup_index(Relation indrel,
  * Don't even think about it unless we have a shot at releasing a goodly
  * number of pages.  Otherwise, the time taken isn't worth it.
+ * Also don't attempt it if we are doing early pruning/vacuuming, because a
+ * scan which cannot find a truncated heap page cannot determine that the
+ * snapshot is too old to read that page.  We might be able to get away with
+ * truncating all except one of the pages, setting its LSN to (at least) the
+ * maximum of the truncated range if we also treated an index leaf tuple
+ * pointing to a missing heap page as something to trigger the "snapshot too
+ * old" error, but that seems fragile and seems like it deserves its own patch
+ * if we consider it.
+ *
  * This is split out so that we can test whether truncation is going to be
  * called for before we actually do it.  If you change the logic here, be
  * careful to depend only on fields that lazy_scan_heap updates on-the-fly.
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to