On Mon, Apr 23, 2018 at 11:21 PM, Amit Kapila <amit.kapil...@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