Clarification on SQLite support

[lukel97]

cc @ldionne

I see there's been a bunch of activity on moving tests to postgres e.g. #197, I think that's fine, and I agree it's better to run these tests on what production servers will run.

But I just want to confirm that we're not removing SQLite support itself? Having the ability to spin up an instance locally without installing postgres is a workflow that myself and a few of my colleagues are currently using. It's handy to view benchmark runs locally.

[ldionne]

@lukel97 Thanks for opening the discussion. My current thinking is that it would be best to indeed drop SQLite support: Supporting two database engines is error prone and has in fact led to numerous bugs, especially in the Postgres support. This means that in order to support SQLite, we often made the unfortunate tradeoff of not properly testing the actual production support, which got broken.

However, I absolutely don't think this should equate with the inability to run lnt locally. I think we should make it easy (even easier if possible) to run lnt locally in a supported and well tested configuration. This could be achieved by lowering the barrier for launching a Docker-based server even more.

Also, out of curiosity, when you spin up a LNT instance locally, what functionality of LNT do you use? Do you only use the one-off comparison tool?

I'm interested to know your thoughts on this as this is central to the direction I'm currently exploring for moving LNT forward.

[lukel97]

I don't think we need to drop SQLite just to support to have better Postgres testing. This seems like a false dichotomy.

Is there a specific postgres-only feature that you're relying on in for the direction you're moving in? I'm happy to only test on postgres and keep that as the only "production" option. But I'd really like to keep the sqlite support in even if it's second class due to how much easier it makes it to run experiments locally.

For local instances it's indeed for comparing individual changes between different clang binaries or flags etc. Mandating docker locally is a no-go for our use case though because we run LNT instances on shared servers without docker installed.

[ldionne]

@lukel97 What kind of comparisons do you do? Just A/B comparisons or do you actually use store multiple runs and want to build historical graphs from that? I want to understand your use case better because I suspect you could be much better served without even needing SQLite.

I don't think we need to drop SQLite just to support to have better Postgres testing. This seems like a false dichotomy.

It's not only about the testing, it's about the complexity of the code generally speaking. If we are serious about supporting both Postgres and SQLite, then we need to have tests for both, which means we have to do some serious refactoring to make it possible to test both using the same test suite. And then there are cases where a feature isn't available with SQLite, such as in the migration in #196 where we can't do the same thing for Postgres and SQLite.

[lukel97]

We are storing multiple runs, that way it's easy to come back and compare e.g. run A which just finished running vs. run B which finished running last week, and may have had a bunch of other runs in between. But I personally don't use the graphing stuff locally, just on cc-perf.igalia.com

It's not only about the testing, it's about the complexity of the code generally speaking. If we are serious about supporting both Postgres and SQLite, then we need to have tests for both, which means we have to do some serious refactoring to make it possible to test both using the same test suite. And then there are cases where a feature isn't available with SQLite, such as in the migration in #196 where we can't do the same thing for Postgres and SQLite.

I'm totally fine with sqlite being supported on a best-effort basis, e.g. I'd be fine with dropping the sqlite checks in the #196 migration. That I can work around just with the sqlite console if needed. And if stuff breaks on main it's not a big deal, I can submit patches whenever to fix that. I can sympathise that database migrations are a pain, and I agree we shouldn't waste time trying to support sqlite's idiosyncrasies.

[ldionne]

@lukel97 Do you folks use the regression detection feature at all? Do you create new regressions using the FieldChanges mechanism? Just trying to figure out what you're using and how.

[lukel97]

@lukel97 Do you folks use the regression detection feature at all? Do you create new regressions using the FieldChanges mechanism? Just trying to figure out what you're using and how.

We use regression detection, but just the automatic mechanism that creates them when a new run is uploaded.

Every morning a cron job then emails the daily report page to us through the lnt send-report command which lists all the regressions.

[ldionne]

Would you be able to send me one of those just so I see what the emails look like? In your experience, are these emails useful?