On Wed, 5 Apr 2023 at 13:42, Andres Freund <andres@anarazel.de> wrote:
>
> Not if you determine a relation specific xmin, and the relation is not a
> shared relation.
>
> ISTM that the problem here really is that you're relying on RecentXmin, rather
> than computing something more accurate. Why not use
> GetOldestNonRemovableTransactionId(rel) - It's a bit more expensive, but I
> don't think it'll matter compared to the cost of truncating the relation.
Thanks for the review!
Hm, I was just copying heapam_handler.c:593 so it would be consistent
with what we do when we create a new table. I wasn't aware we had
anything that did this extra work I'll look at it.
But I'm not sure it's the best idea to decide on how
truncate/vacuum/create table work based on what happens to be easier
to test. I mean I'm all for testable code but tieing vacuum behaviour
to what our test framework happens to not interfere with might be a
bit fragile. Like, if we happen to want to change the testing
framework I think this demonstrates that it will be super easy for it
to break the tests again. And if we discover we have to change the
relfrozenxid behaviour it might be hard to keep this test working.
> Somehow it doesn't feel right to use vac_update_relstats() in
> heapam_handler.c.
>
> I also don't like that your patch references
> heapam_relation_nontransactional_truncate in AddNewRelationTuple() - we
> shouldn't add more comments piercing tableam than necessary.
I'll take another look at this tomorrow. Probably I can extract the
common part of that function or I've misunderstood which bits of code
are above or below the tableam.
I think fundamentally the hardest bit was that the initial
relfrozenxid bubbles up from heapam_handler.c via a return value from
set_new_filelocator. So unless I want to add a new tableam method just
for relfrozenxid it's a bit awkward to get the right data to
AddNewRelationTuple and vac_update_relstats without duplicating code
and crosslinking in comments.
> To me it seems important to test that your change actually does what it
> intends to. Possibly the test needs to be relaxed some, but I do think we want
> tests for the change.
I missed the comment about relaxing the tests until just now. I'll
think about if there's an easy way out in that direction too.
If it's cutting it too fine to the end of the commitfest we could
always just commit the warnings from the 001 patch which would already
be a *huge* help for admins running into this issue.
Chag Sameach!
--
greg