On Thu, 1 Dec 2022 at 14:18, Andres Freund <andres@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