On Thu, 1 Dec 2022 at 14:18, Andres Freund <and...@anarazel.de> wrote: > > I find it problematic that ResetVacStats() bypasses tableam. Normal vacuums > etc go through tableam but you put a ResetVacStats() besides each call to > table_relation_nontransactional_truncate(). Seems like this should just be in > heapam_relation_nontransactional_truncate()?
So this seems a bit weird. The only other part of the tableam that touches freezexid and minmxid is table_relation_set_new_filelocator() which returns them via references so that the callers (heap.c:heap_create() and relcache.c:RelationSetNewRelfilenumber()) can set them themselves. I can't really duplicate that layering without changing the API of heapam_relation_nontransactional_truncate(). Which if it changes would be quite annoying I think for external pluggable tableams. But you're right that where I've put it will update relfrozenxid and minmxid even for relations that have a tableam handler that returns InvalidXid and doesn't need it. That does seem inappropriate. I could put it directly in the heapam_handler but is that a layering issue to be doing a catalog update from within the tableam_handler? There are no other catalog updates in there. On the other hand the existing callers of *_nontransactional_truncate() don't have any existing catalog updates they want to make so perhaps returning the xid values by reference was just for convenience to avoid doing an extra update and isn't needed here. -- greg
