Now using DISTINCT in ivoid-selecting registry subqueries.

[msdemlei]

This is not only more logical (let's filter out duplicates as early as possible)but experimentally also is enough to keep the planner going off to follies of the type encountered in bug #571.

[msdemlei]

I think no-changelog-entry is roughly right, although I have added the new PR number to the changelog entry about subqueries (that doesn't seem to placate the CI, though).

Anyway, the current test failure appears to be unrelated to the change at hand (though I'd say we should do something about the sia2 tests in general, as they generally take ages). So, I think this is ready to go.

[ManonMarchand]

I was unsure about the changelog. But I guess it can be merged with the failure.

I don't have time to review today but I'll look tomorrow.

These two successive PRs make me wonder how we could detect that a change that was done makes everything slower. Would it be wise to benchmark?

[msdemlei]

On Tue, Jul 02, 2024 at 05:46:43AM -0700, Manon Marchand wrote: These two successive PRs make me wonder how we could detect that a change that was done makes everything slower. Would it be wise to benchmark?

I'm afraid this is one of the many things where we can't sensibly defend with unit tests or something like them. The problem here was, at its core, a specific fluke on one specific service. Ok, it's the default one we use, but still: if we try to even diagnose performance regressions, we will have useless test suites because (a) they'll take forever to finish and (b) they'll only pass once every blue moon because the more services you hit the more certain it becomes that at least one of them will be down or misbehaving at any time. I'm still all for a wider array of tests, perhaps also human-executed tests (I'd be in for checking a notebook or two), to be executed before we do a release. This would have caught this problem, but frankly, that would mainly have been sheer luck, too.

[bsipocz]

I was unsure about the changelog. But I guess it can be merged with the failure.

adding the PR number is certainly the preferred way for the changelog for this. Or calling it "affect-dev", and when that label present the checker expects no changelog entry.

The reason for the CI job to fail was the missing milestone though.

[bsipocz]

(though I'd say we should do something about the sia2 tests in general, as they generally take ages)

That's temporary and is due to cadc experiencing some server side issues.

[bsipocz]

Looks good to me, and I can confirm the speed improvement compared to main (and even main was speedier than what we had in the last release, so I cannot really benchmark this further)

[andamian]

I think that the extra resources required by DISTINCT (temporary table to sort and remove duplicates) are saved with UNION ALL so overall the impact on the database is small.