Scaffold index operators and included (covered) index columns#709
Scaffold index operators and included (covered) index columns#709roji merged 2 commits intonpgsql:devfrom
Conversation
roji
force-pushed
the
index-scaffolding-stuff
branch
from
5ed5804 to
1827dde
Compare
austindrenski
left a comment
austindrenski
left a comment
There was a problem hiding this comment.
Looks good. One real question on the version detection in testing, otherwise just some nits.
| [Fact] | ||
| public void Index_covering() | ||
| { | ||
| if (Fixture.TestStore.GetPostgresVersion() < new Version(11, 0)) |
There was a problem hiding this comment.
Just a note, but should we at some point start testing against multiple release versions of PostgreSQL?
Might be something to consider in a few weeks, after 2.2 and the move to Azure Pipelines.
There was a problem hiding this comment.
A long, long time ago it actually worked this way. The problems were a much longer build time (multiplied by the number of versions supported), and significantly more complexity in maintenance of the tests themselves and/or the CI platform (this may be partially gone with Azure, or not).
As a general rule, I'm not sure we've seen many version-related issues that weren't reported and fixed rather quickly. So while testing all versions is theoretically better, I'm simply not sure the added value is worth the downsides... Am definitely open to discussing this though - maybe open a new issue for discussing?
|
@austindrenski thanks for reviewing so quickly! Pushed a commit fixing the nits, and see my comments on the larger issues of versioning. |
roji
force-pushed
the
index-scaffolding-stuff
branch
from
a230366 to
79f86da
Compare
3 participants
Closes #704
/cc @khellang