DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
8 stars 2 forks source link

Make ClassDB compatibile with Postgres versions prior to 9.6 #227

Closed smurthys closed 6 years ago

smurthys commented 6 years ago

All scripts need to be tested in different versions of Postgres from 9.3 to 9.5 to verify compatibility status with Postgres versions prior to 9.6. Ideally, ClassDB should be made compatible with Postgres 9.3. Where compatibility is not possible, workarounds should be explored and documented.

wildtayne commented 6 years ago

Not sure how much progress has been made on these issues, however it looks like Postgres 9.3 goes EOL in September (support timeline). Considering this, it may not be worth adding 9.3 specific workarounds (depending on how much effort it will be).

smurthys commented 6 years ago

Good point @srrollo about EOL of pg 9.3. However, I think it makes sense to support it because we should expect those still using it to use it for another year or so.

Of course, the choice also depends on how much effort it is to make ClassDB compatible with 9.3. One way to think about this might be that 9.3 compatibility is lowest priority among all compatibility issues.

KevinKelly25 commented 6 years ago

As seen in #233 there is a chance that this epic is starting to grow beyond the scope and time frame of M3.

We already have 5 outstanding issues in this epic and although we have made progress on those issues the amount of dev effort required for the full compatibility of pre-9.6 versions keeps rising. Also, there are large parts of ClassDB we have not been able to implement on 9.3 because of the outstanding issues. I have a suspicion that after we fix some or all of these issues that other issues are going to pop up in those portions of code that we have not been able to get to yet.

With that in mind and that we are trying to get M3 done relatively soon, I suggest that we table the epic for a later milestone so we can put more priority into the main goals of M3. I am curious to know other opinions on if we should include this in M3, a later milestone or that it isn't worth doing at all.

smurthys commented 6 years ago

Recapping the key lesson from PR #233 that static SQL code cannot use any unsupported feature/syntax even if the code is conditionally executed only on supported versions. This means parts of scripts need to be executed either dynamically or be duplicated to create version-specific code. The former is approach is bearable in some cases; the latter should be avoided in most cases due to maintenance concerns.

With that lesson in the background, we need to immediately answer at least the following questions.

  1. Should pre9.6 compatibility be in M3? Or, is it better create an M4 for this summer with only pre9.6 compatibility as a goal?
  2. Taking a cue from @srrollo's comment earlier, should we put the effort to support pg9.3 given that it reaches EOL in this September (2018)?
  3. Long term, what is our strategy to support different pg versions? A simple approach is to assure support as long as the DBMS is supported.

I believe we should not address any server portability issues until we have answered these and related questions here. The only issue we should address is #230 because it is a trivial fix and PR #233 already exists. (No point in wasting the effort already invested.)

PS: As I type this comment, I see @KevinKelly25 just commented opining on my Question 1 in this comment.

afig commented 6 years ago
  1. I agree with @KevinKelly25 that it would be better to not do this in M3. We did not anticipate a large number of changes when originally deciding to place it into M3, nor the difficulties involved in implementing version-specific code. I side with pushing this off for a later release, likely in an M4 milestone later this summer.

  2. I would say yes, since once we have a method of implementing version specific code, adding support for 9.3 would not be significantly more difficult than other versions.

  3. I think a strategy that would work is to ensure compatibility with versions that are supported at the time of the version's release. This means that M4, if done this summer, should support pg9.3. However, M5, which will likely be completed after September 2018, does not need to support pg9.3. Hot-fixes that are not a part of a new milestone should support the versions of Postgres versions that the previous milestone supported.

Overall, I think a major improvement that would make this this effort significantly easier is to make testing different versions of Postgres easier, and perhaps even automated (though it would take some time to study/design how this would be done, it would save much time in the future, and would be an interesting learning experience). This would likely eliminate most of the issues involved with testing and debugging dynamic code, since there should be at least one attempt to execute the code.

wildtayne commented 6 years ago

I pretty much agree with @afig 's assessment. I think the decision to support 9.3 will depend on when M4 is released, if it is during the summer it should support 9.3.

I think an automated version test is a great idea. I can image a psql test script that test multiple versions by leveraging the fact that multiple instances installed on the same machine automatically configure themselves to listen on different ports. The script would then be given a port range and run the tests once per port (one connection per port). I imagine the version could even be extracted using our help function, so there does not even need to be an exact map from port to version.

smurthys commented 6 years ago

Thank you all for your thoughts. Here are my thoughts (some of which others may have already mentioned).

Yes to creating Milestone 4 to address compatibility (exclusively) soon after M3 is released.

We should definitely support 9.4 and 9.5.

I feel we should exclude support for 9.3. I worry we are underestimating the effort to support 9.3 because the difference in the GRANT syntax (see #229) and a lot of our code is about granting/revoking. The development, testing, and documentation effort cannot be justified for a version with a short lifespan.

In general, as @afig says, our policy should be to support all DBMS versions which are themselves still supported. However, we should probably say that except under special circumstances (such as hot fixes), we will not spend development effort to support a DBMS version whose EOL is within 90 days (a magic number).

In every DBMS version we claim to support, we should support 1st party distro for both Windows and Linux, as well as a 3rd party distro from Ubuntu/CentOS. To achieve this, we need to collectively (among us) be able to run the variety of distros. However, we don't need to test on every version-distro combination. For each distro, we need to test only the highest DBMS version for which a version-specific implementation exists. For example, assume some DBMS feature exists in 9.4 and 9.5 and a different way of doing that exists in 9.6. Then we need to test 9.5 (highest of 9.4 and 9.5) and 9.6.

Also, if a feature in a DBMS version gets an "easier" version in a later version but the feature in the earlier version still works in the newer version, we should just use the feature in the earlier version, unless a major benefit is seen due to using the feature in the newer version. For example, Issue #230 should be resolved using the solution that works in 9.4 because it also works in later versions.

Where possible, we should remove version-specific code when we no longer support a DBMS version. For example, the 9.4 solution used to solve Issue #230 should be replaced with the 9.5 solution when 9.4 is no longer supported. (This is probably not a very good example, because there won't be any version-specific code in this case, but I hope the idea is communicated.)

We need to figure out a way to mark code regions that are version-specific or somehow require attention due to versions so they can be found easily and edited/removed.

KevinKelly25 commented 6 years ago

@smurthys Thank you for the overview of the situation. I completely agree with your analysis.

I do see one issue though. As mentioned here

I feel we should exclude support for 9.3. I worry we are underestimating the effort to support 9.3 because the difference in the GRANT syntax (see #229) and a lot of our code is about granting/revoking. The development, testing, and documentation effort cannot be justified for a version with a short lifespan.

The Grant syntax was not changed until 9.5 which means that 9.4 would have the same issues as 9.3 in this aspect. Since we are supporting 9.4 we might be inherently supporting a big chunk of 9.3 as well. Alternatively, if it is a huge development issue because of the grant change we may have to consider not supporting 9.4 as well. Either way, it is something we have to discuss more.

smurthys commented 6 years ago

@KevinKelly25 Thanks for your comment. I misinterpreted the title and the description of Issue #229 as saying the GRANT syntax changed from 9.3 to 9.4. (Perhaps that issue's title should be revised for clarity.)

I still think it is OK not to specifically address 9.3 due to its impending EOL: falls under the "90 day" policy I proposed. At the least, we need to give it low priority.

afig commented 6 years ago

Thanks for the great summary @smurthys. Just wanted to add my thoughts about each topic mentioned:

I do not think we are underestimating the effort that the change in syntax causes. I left a comment in #229 about this. As @KevinKelly25 mentioned, the change also affects 9.4, so it turns out we should still make an effort to support it regardless of difficulty.

I agree that we should generally not support versions that are near EOL unless no changes are needed (Which is to say, we should still test ClassDB on a near-EOL version, but if it turns out a change is needed, then we should just say that the version is not supported.)

I also agree that we should run tests on those specified distributions of Postgres, along with the fact that we do not need to test every combination.

I think avoiding the use of easier but newly introduced features will get easier over time as we become familiar with each version of Postgres and start testing with older versions. Currently, it is difficult to know when exactly a feature was introduced without specifically coming across an issue caused by it.

Removing version specific code sounds good as well. Marking code regions that are version-specific would also be good. We should also figure out a special identifier for upgrade-specific code.

smurthys commented 6 years ago

Thanks @afig for the note on effort.

Given that compatibility will not be addressed in M3, at this point, it looks like we can suspend this conversation until after the release of M3. Also, we should remove M3 designation from issues related to compatibility.

smurthys commented 6 years ago

I added a wiki page to make a list of items needing attention related to Postgres compatibility. Please update the page as we encounter items so we can get a full picture at one place.

I have also updated the sidebar on the Wiki home page so the new page is easily accessed.

Please do not create new issues related to Postgres compatibility until after M3 is complete and we have had a chance to review the items on the Wiki page.

michaeltorres1 commented 6 years ago

I am aware that version 2.2 has been released and I have been reading through ClassDB and noticed that this issue #227 is still opened but I believe it should have been closed.

afig commented 6 years ago

Thanks for pointing this out @michaeltorres1. This issue should have indeed been closed sometime during the day that ClassDB 2.2 was released (likely after system testing had passed).