Allow SQLT options to be passed to unique constraints #87
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ther time Read diff under -w
It is not 100% clear to me why this is happening, as the change discussed[1] was not supposed to change behaviour outside of a transaction. The test in question essentially does a blocking request with a timeout. The way the default DBIC machinery works is to issue a ping() after the timeout induced exception, in order to opossibly retry the operation IFF the link went stale ($dbh thinks everything is in order, the RDBMS believes otherwise). After the changes in 3.5.0 this ping() now hangs, even though the access pattern did not change (there are no transactions involved on the handle in question). Either way, the fix is simple: just shim-away the ping for the duration of the test. In general this should be resolved on a higher level by disabling retries for the duration of the test (or maybe even disabling retries by default in the distant feature - see RT#47005) [1] https://rt.cpan.org/Ticket/Display.html?id=100648#txn-1438093
…arounds ARGH! In the end the issue turned out to be stupid-simple: when PadWalker is called it always returns us a hashref. This hashref is traversed, with its own address recorded as seen. Once processed, the hashref itself is GCed, leaving a stale entry in $seen_refs. Up until 5.18 this was not a problem, likely due to no address reuse. However after 5.18 PadWalker is very happy to return different hashrefs under the same address over and over again. The $seen_refs entry leads to a skip, and the actual contents of the pad are never examined - we've seen this hash already. ARGH! The decisive change is the single line around PadWalker - everything else is cleanup of workarounds for this problem.
Switch synopsis away from M::I (it still scares people, boggle) Document volatility of req_group_list Better language in places Coarser granularity of heredocs (easier to read) Read changes under -w
Requires a one-time break of req_group_list() Also tighten up the optdep spec and handle conflicting versions saner As a side effect fix 34d2dea to be truly copy-paste-able (no , separator) Changes to ::Opt::Deps and xt/optional_deps.t best viewed under -w
Leaving the old method in place indefinitely - may be used in the wild
…arations
No logical changes: verified before and after the commit via:
perl -MData::Dumper::Concise -Ilib -MDBIx::Class::Optional::Dependencies -e '
my $d = DBIx::Class::Optional::Dependencies->req_group_list;
delete $d->{$_} for grep /rdbms_generic/, keys %$d;
delete $_->{effective_modreqs_differ} for values %$d;
print Dumper $d
' | sha1sum
Read under -w
More in the following commit
This is the meat of this refactor, inspired by multiple parties complaining (with good reason) about the sluggishness of our skip_all()s, and by groundwork in both c8dc7d3 and 2c2bc4e (both never applied - the former is superseded by a more direct approach and the latter being too intrusive with too little benefit after the refactor) Only tests with unambiguous skips were switched over - later commits will address multi-rdbms loops in a different manner. The loss of skip-time warnings "this test creates tables blah and blah" was deliberate, and due to several reasons *) They are displayed at the wrong time *) They are not well sycnrhonized with what the actual test does *) Some time this year all table names will switch to dbictest_-prefixing All in all the thinking is that nothing of value is lost, and the changeset is greatly simplified as a result. The loading before strict/warning in tests is actually significant. It appears that in "all deps fail" cases the speedup of not loading strictures is quite significant (demonstrated below difference on WiP CDBICompat streamlining): strictures first: Files=43, Tests=0, 1 wallclock secs ( 0.14 usr 0.08 sys + 0.35 cusr 0.15 csys = 0.72 CPU) optdeps first: Files=43, Tests=0, 1 wallclock secs ( 0.13 usr 0.06 sys + 0.14 cusr 0.11 csys = 0.44 CPU)
The test_rdbms_msaccess_* groups are only referenced in two tests. The basic one does not exercise (doesn't even load) any of the ICDT handling components The ICDT test already explicitly requires test_dt, so no changes are needed within the actual tests The erroneous requirement was introduced in 726c8f6 and never revised since
Read diff/blame with -C -C -M -w
The deterministic_value test checks how stringification behaves wrt inherited values from a resultset. Rename test while we are at it. The plus_select.t checks behavior of InflateColumn combined with get_columns. In both cases the column in question (year) is declared as VarChar and is not connected with IC::DT in any way Read under -C -C -M -w
Even though this is a hard test_requires (for now), move things to a proper specification within OptDeps like all other RDBMS (will make icdt specs much easier). The OptDep module is very lightweight anyway, and loading it on all installs will be a good exercise before spinning it off.
They will be filled in during the next commits, done separately to reduce diff-noise
A group can specify not only what it requires (with includes), but can also request a specific set of modules IFF another group is required within the same call. This allows to do e.g. IC::DT+RDBMS dependencies in a sane manner. Delete test_dt* groups from our optdeps (they were never documented, and a cpangrep does not show any use) - instead we replace them with an elaborate network of augments.
On rare occasions tests need a specific module version, but nothing else in the dist needs this. Additionally only parts of a test may rely on this extra requirement, making the "all or nothing" approach of -skip_all_without not very attractive. Hence introducing ability to supply arbitrary module specifications in lieu of group names. Each specification is checked against a special group 'test_adhoc' which ensures that optional deps are not "forgotten" within the suite (causing a test to never run in practice).
A use() is compile-time even when it is in an INIT block, hence removing the INITs is safe. Do not do it for require() invocations, as it is not clear if the tests rely on the specific order or not (more sleeping dogs left to lie) Read under -w
f56e59c added the extra module, but nothing in the tests actually loads it under Film (there likely was a switch during dev, but it was never committed). Just rip it out entirely.
This work is a direct descendant of ilmari's c8dc7d3, even though it does not contain almost any of the original code. While the original approach was a big improvement, this commit is taking advantage of all improvements that took place in OptDep handling in earlier commits. The result (at the expense of a much much larger changeset) is a skip time of < 0.5s for the entire cdbi set ilmari++ # this revamp would not have taken place without your first nudge
…ld perl The *really* old version is needed to make sure we end up upgrading as much stuff as possible, thus hopefully trapping more dev-rel problems before they end up on CPAN ( absence of this entry caused [email protected] to slip by unnoticed )
Tighten up code while at it, no functional changes intended
There is no need to call out all the way to _extract_order_criteria - we can do the necessary mangling ourselves
No functional changes at all, just more efficient loops Read under -w
More surprising stuff showing up on the profiles...
Speeds up the analyzer massively, now it's mostly SQLA itself holding things back... sigh
Instead reuse a previously calculated one, since the code flow allows it There *seem* to be no functional changes, but one never knows...
No longer try to match individual pieces at all, simply treat every single fragment as a single string. While rather barbaric, we are doing it anyway in order to catch fragments, thus it makes no sense to keep the "proper" codepath at all. This concludes the shenanigans in this function
This discussion comes up several times per year, just put it to bed for good
This not only fixes an obscure test failure due to an older Sub::Uplevel (it would be too obnoxious to bump the dep just for the test case), but also makes the entire codebase more robust in light of possible rogue/incomplete caller() overrides This was not a simple s/// job - each change was manually evaluated before carrying out Also while at it - fix the utterly annoying *UNKNOWN* eception site-marker: it does not add any information and only confuses things. The heuristics in Carp::Skip is supposed to be much much clearer, need to finish its tests...
…rabs I am not sure why I wrote things this way back in 8d6b147, perhaps I didn't properly think through that sqlt_type will be correctly determined no matter what. As a result any MySQL test would connect at least once, and under the right circumstances consume enough per-connection-thread memory to get a low-RAM system (like TravisCI) to start OOM-killing things indiscriminately.
…ocking Without this too many connections are fired at the MySQL and tasty OOMs occur
Given the very low amount of RAM, the travis setup DOES NOT HELP. Instead of fishing out the changed configs, just symlink the dirs back where they belong
Although in reality it is the preceeding commits that make the real difference
…evrel This is not strictly necessary for anything I do, just add even more smoke coverage to some pieces of the ecosystem (at the expense of extra 5 minutes, but given it's in the "fail-ok" blocks - meh)
…g-doubles This one to catch fallout of https://rt.perl.org/Ticket/Display.html?id=72210
…oint hell Issues similar, but likely not limited to, P5#72210 ( see prev commit)
Altreus
force-pushed
the
people/altreus/unique-options
branch
from
c958617 to
f679481
Compare
Altreus
force-pushed
the
people/altreus/unique-options
branch
from
f679481 to
05e063c
Compare
|
Killing this after a rebase |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does work against the current version of SQLT, but that version of SQLT doesn't actually do anything with the information.
There is a PR on SQLT that will actually cause this to have some effect.
dbsrgits/sql-translator#75
I will try to write some tests for this, but I wanted to get the PR made so I can invite discussion in the meantime.