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 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. The new arguments to heap_prepare_freeze_tuple() need to be documented in its header comment. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
