Hi, On 2023-04-05 13:26:53 -0400, Greg Stark wrote: > On Wed, 5 Apr 2023 at 11:15, Andres Freund <and...@anarazel.de> wrote: > > > > The freebsd test that failed is running tests in parallel, against an > > existing > > cluster. In that case it's expected that there's some concurrency. > > > > Why does this cause your tests to fail? They're in separate databases, so > > the > > visibility effects of the concurrent tests should be somewhat limited. > > Because I'm checking that relfrozenxid was updated but any concurrent > transactions even in other databases hold back the xmin.
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. 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. > Honestly I'm glad I wrote the test because it was hard to know whether > my code was doing anything at all without it (and it wasn't in the > first cut...) But I don't think there's much value in having it be in > the regression suite. We don't generally write tests to ensure that a > specific internal implementation behaves in the specific way it was > written to. 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. Greetings, Andres Freund