On Fri, Apr 27, 2018 at 12:18 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 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.
>
Hmm, in some rare cases when OID will be reused, it can still lead to
problems (when Create Index is performed in parallel to Cluster) as
mentioned by Andrew. I am not sure if that is acceptable on the
grounds that it is not a newly introduced problem by his proposed
patch. I think if we can come up with a solution which prevents
OldestXmin from retreating, it will be good, otherwise, we might have
to think harder whether there is any other solution to this problem
which will attack the root cause of the problem and if nothing better
comes out, then probably we can go with his proposed patch.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com