The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested
I have read this patch. I like the concept and would like it to get committed.
Question I have after reading the patch is around this construct:
/*
- * If there are no indexes then we can vacuum the page right now
- * instead of doing a second scan.
+ * If there are no indexes we can vacuum the page right now
instead of
+ * doing a second scan. Also we don't do that but forget dead
tuples
+ * when index cleanup is disabled.
*/
This seems to change behavior on heap tuples, even though the option itself is
documented to be about "Indexes" only. This needs either better explanation
what "forget dead tuples" means and that it does not lead to some kind of
internal inconsistency, or documentation on what is the effect on heap tuples.
This same block raises a question on "after I enable this option, do a vacuum,
decide I don't like it, what do I need to do to disable it back?" - just set it
back, or set and perform a vacuum, or set and perform a VACUUM FULL as
something was "forgotten"?
It may happen this concept of "forgetting" is documented somewhere in the near
comments but I'd prefer it to be stated explicitly.
The new status of this patch is: Waiting on Author
