On Mon, Mar 15, 2021 at 12:58 PM Peter Geoghegan <p...@bowt.ie> wrote: > > I'm not comfortable with this change without adding more safety > > checks. If there's ever a case in which the HEAPTUPLE_DEAD case is hit > > and the xid needs to be frozen, we'll either cause errors or > > corruption. Yes, that's already the case with params->index_cleanup == > > DISABLED, but that's not that widely used. > > I noticed that Noah's similar 2013 patch [1] added a defensive > heap_tuple_needs_freeze() + elog(ERROR) to the HEAPTUPLE_DEAD case. I > suppose that that's roughly what you have in mind here?
I'm not sure if you're arguing that there might be (either now or in the future) a legitimate case (a case not involving data corruption) where we hit HEAPTUPLE_DEAD, and find we have an XID in the tuple that needs freezing. You seem to be suggesting that even throwing an error might not be acceptable, but what better alternative is there? Did you just mean that we should throw a *better*, more specific error right there, when we handle HEAPTUPLE_DEAD? (As opposed to relying on heap_prepare_freeze_tuple() to error out instead, which is what would happen today.) That seems like the most reasonable interpretation of your words to me. That is, I think that you're saying (based in part on remarks on that other thread [1]) that you believe that fully eliminating the "tupgone = true" special case is okay in principle, but that more hardening is needed -- if it ever breaks we want to hear about it. And, while it would be better to do a much broader refactor to unite heap_prune_chain() and lazy_scan_heap(), it is not essential (because the issue is not really new, even without VACUUM (INDEX_CLEANUP OFF)/"params->index_cleanup == DISABLED"). Which is it? [1] https://www.postgresql.org/message-id/20200724165514.dnu5hr4vvgkssf5p%40alap3.anarazel.de -- Peter Geoghegan
