Document that vacuum can't truncate if old_snapshot_threshold >= 0
Hi,
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 / REL_TRUNCATE_FRACTION) &&
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.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 13, 2016 at 02:14:06PM -0700, Andres Freund wrote:
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.
[Action required within 72 hours. This is a generic notification.]
The above-described topic is currently a PostgreSQL 9.6 open item. Kevin,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20160527025039.GA447393@tornado.leadboat.com and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.
[1]: /messages/by-id/20160527025039.GA447393@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jul 15, 2016 at 02:38:54AM -0400, Noah Misch wrote:
On Wed, Jul 13, 2016 at 02:14:06PM -0700, Andres Freund wrote:
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.[Action required within 72 hours. This is a generic notification.]
The above-described topic is currently a PostgreSQL 9.6 open item. Kevin,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1. Consequently, I will appreciate your
efforts toward speedy resolution. Thanks.[1] /messages/by-id/20160527025039.GA447393@tornado.leadboat.com
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:
/messages/by-id/20160527025039.GA447393@tornado.leadboat.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jul 13, 2016 at 4:14 PM, Andres Freund <andres@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 / REL_TRUNCATE_FRACTION) &&
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
Attachments:
sto-no-truncation.difftext/plain; charset=US-ASCII; name=sto-no-truncation.diffDownload
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'
</para>
<para>
+ 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 b/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.