Hi,
On 2023-04-05 13:26:53 -0400, Greg Stark wrote:
> On Wed, 5 Apr 2023 at 11:15, Andres Freund <andres@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