On Mon, Apr 23, 2018 at 11:21 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> If I read correctly, it seems one of the main reason [1] is to save
> the extra pass over the heap and improve the code. Now, ideally, the
> extra pass over heap should also free up some space (occupied by the
> rows that contains old toast pointers corresponding to which we are
> going to remove rows from toast table), but it is quite possible that
> it is already clean due to a previous separate vacuum pass over the
> heap. I think it is good to save extra pass over heap which might not
> be as useful as we expect, but that can cost us correctness issues in
> boundary cases as in the case being discussed in this thread.
I don't think anybody disagrees with that, but it doesn't answer the
question of how best to fix it. Making VACUUM a lot more expensive
will not win us any awards, and an extra pass over the heap could be
very expensive. You seem to think Andrew's fix isn't really
addressing the root of the problem, but I think that just depends on
how you define the problem. If you define the problem as "the table
should never have dangling pointers to the TOAST table", then Andrew's
approach is only fixing the symptom. But if you define the problem as
"we shouldn't try to follow TOAST pointers in heap tuples that are not
visible to any current or future MVCC snapshot", then it's fixing the
root issue.
I wonder if perhaps get_actual_variable_range() has a hazard of this
type as well. If OldestXmin retreats, then the special snapshot type
which it uses could see a row whose TOAST entry has been removed
meanwhile.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company