On Fri, Aug 28, 2020 at 9:49 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Tue, Jul 21, 2020 at 9:21 AM Dilip Kumar <dilipbal...@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
v4-0001-Provide-a-GUC-to-allow-vacuum-to-continue-on-corr.patch
Description: Binary data
