Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING - Mailing list pgsql-hackers

From Dilip Kumar
Subject Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING
Date
Msg-id CAFiTN-t9_rCqw_B-U_19f-srA7MbSGKdZi6abRO4fY6Lb2WK7w@mail.gmail.com
Whole thread Raw
In response to Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
On Fri, Aug 28, 2020 at 9:49 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Jul 21, 2020 at 9:21 AM Dilip Kumar <dilipbalaut@gmail.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

Attachment

pgsql-hackers by date:

Previous
From: Andrey Lepikhov
Date:
Subject: Re: Ideas about a better API for postgres_fdw remote estimates
Next
From: Dilip Kumar
Date:
Subject: Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING