Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING

On Fri, Aug 28, 2020 at 9:49 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Jul 21, 2020 at 9:21 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > In the previous version, the feature was enabled for cluster/vacuum
> > full command as well. in the attached patch I have enabled it only
> > if we are running vacuum command. It will not be enabled during a
> > table rewrite. If we think that it should be enabled for the 'vacuum
> > full' then we might need to pass a flag from the cluster_rel, all the
> > way down to the heap_freeze_tuple.
>
> I think this is a somewhat clunky way of accomplishing this. The
> caller passes down a flag to heap_prepare_freeze_tuple() which decides
> whether or not an error is forced, and then that function and
> FreezeMultiXactId use vacuum_damage_elevel() to combine the results of
> that flag with the value of the vacuum_tolerate_damage GUC. But that
> means that a decision that could be made in one place is instead made
> in many places. If we just had heap_freeze_tuple() and
> FreezeMultiXactId() take an argument int vacuum_damage_elevel, then
> heap_freeze_tuple() could pass ERROR and lazy_scan_heap() could
> arrange to pass WARNING or ERROR based on the value of
> vacuum_tolerate_damage. I think that would likely end up cleaner. What
> do you think?

I agree this way it is much more cleaner. I have changed in the attached patch.

> I also suggest renaming is_corrupted_xid to found_corruption. With the
> current name, it's not very clear which XID we're saying is corrupted;
> in fact, the problem might be a MultiXactId rather than an XID, and
> the real issue might be with the table's pg_class entry or something.

Okay, changed to found_corruption.

> The new arguments to heap_prepare_freeze_tuple() need to be documented
> in its header comment.

Done.

I have also done a few more cosmetic changes to the patch.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com