rgeo / activerecord-postgis-adapter

ActiveRecord connection adapter for PostGIS, based on postgresql and rgeo
Other
870 stars 238 forks source link

When using parallel tests out of the box post gis spatial ref table isn't copied over to test databases. #342

Closed joegaudet closed 3 years ago

joegaudet commented 3 years ago

Hey!

So I've observed that in addition to the truncation issue which you've kindly resolved. When an app is configured with activerecord-postgis-adapter and parallelized, the spatial ref table isn't copied over on initial setup. I worked around this by including the setup file and checking for its presence beforehand, but.... it's less than ideal.

I observed this using the latest rails + minitest with parallel test enabled as per the guides:

https://guides.rubyonrails.org/testing.html#parallel-testing

keithdoggett commented 3 years ago

So I looked into this and the problem is that after the database is forked, it sets up the database with the load_schema method. Since we ignore all of the spatial tables they aren't copied over after the fork.

Do you know if PostGIS is installed on the forked database? The fix might be as simple as adding an after_fork_hook.

joegaudet commented 3 years ago

It is yes!

My temporary fix was to check-in the SQL file that populates that table and load it if need be. But that felt a bit like a hack :P

.joe

On Mon, May 3, 2021 at 5:52 AM Keith Doggett @.***> wrote:

So I looked into this and the problem is that after the database is forked, it sets up the database with the load_schema method. Since we ignore all of the spatial tables they aren't copied over after the fork.

Do you know if PostGIS is installed on the forked database? The fix might be as simple as adding an after_fork_hook.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rgeo/activerecord-postgis-adapter/issues/342#issuecomment-831239055, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA6IBZKZHTWCP2AJUJ3FLDTL2MB3ANCNFSM43V3URSQ .

joegaudet commented 3 years ago

@keithdoggett this is the approach I used, but it required me to check-in the .SQL file which is not really great, is there a command I could run instead?

  parallelize_setup do |worker|
    # The parallel runners don't seem to always bring along the spatial table
    # If the spatial table has been nuked for some reason load it back up.
    if ActiveRecord::Base.connection.execute('select * from spatial_ref_sys').count == 0
      ActiveRecord::Base.connection.execute(IO.read("db/spatial_ref_sys.sql"))
    end
  end
keithdoggett commented 3 years ago

Thanks that's helpful. I haven't had time to look into this more, but I think we may have to add a custom hook into the adapter that either installs PostGIS again to redo the setup or copies the spatial_ref_sys table over from the main database.

joegaudet commented 3 years ago

Yeah makes sense, let me know if I can help anyway, I'm sorta blocked on this so happy to collaborate if you could point me in the right direction?

If there's a command that I could invoke that sets it up that'd probably be enough.

I'm not sure how I'd get a handle on the main table to copy it over tho.

keithdoggett commented 3 years ago

So the databases are created by running the after_fork_hook block in this file (https://github.com/rails/rails/blob/main/activerecord/lib/active_record/test_databases.rb). Which ends up calling the reconstruct_from_schema method here (https://github.com/rails/rails/blob/91f0db1353e1b24e2d26ebbbadce510735f66848/activerecord/lib/active_record/tasks/database_tasks.rb#L404-L420).

I'm not sure exactly the best way to handle this. I think it would be to override create_and_load_schema in the first file and add a new method to copy over the spatial_ref_sys table from somewhere. Then call that after reconstruct_from_schema is called.

I guess the big thing would be to figure out how to load in spatial_ref_sys. Could check it in to the repo here, similar to how you're doing it or we could copy it from the main testing database somehow.

joegaudet commented 3 years ago

@keithdoggett I would assume we'd want to copy it, since there's some possibility it changes in the future right?

keithdoggett commented 3 years ago

I was able to dig into this a little more and realized that I actually couldn't reproduce this while building off of master (all of the forked databases had the full table). Then I realized that #338 actually fixes this as well. This method is called when the forked databases are created and properly sets up PostGIS via the setup_gis call.

https://github.com/rgeo/activerecord-postgis-adapter/blob/f7108d4456bf36b82b8340f69ab19aed976bb073/lib/active_record/connection_adapters/postgis/postgis_database_tasks.rb#L21-L33

The issue is that truncate_tables would delete the contents of spatial_ref_sys each run, but this has been fixed now. Can you test this out by deleting the forked databases that were created and installing this gem from the master branch since that has #338 merged?

You can build from master in your Gemfile like so:

gem 'activerecord-postgis-adapter', git: 'git@github.com:rgeo/activerecord-postgis-adapter.git', branch: 'master'
joegaudet commented 3 years ago

I'm seeing that here now, thanks @keithdoggett

joegaudet commented 3 years ago

@keithdoggett the plot thickens! I only observe this truncation when I run the tests inside of rubymine/intellij. When I run them from the console, it works fine.

mjy commented 3 years ago

Is it Spring related? We have hit a problem that we've isolated to a Spring issue that was first detected by RM vs. terminal rspec commands.

joegaudet commented 3 years ago

@mjy You suggesting we try disabling spring?

keithdoggett commented 3 years ago

Or at least restart it. Sometimes spring uses cached versions of gems, so you might inadvertently still be running the old version of the gem.

joegaudet commented 3 years ago

Ohhhh yes that makes sense.

mjy commented 3 years ago

@joegaudet we found that with Spring running we have a Factory problem, it maybe completely unrelated, but the fact that you are doing something in RM, and RM prefixes with Spring had me wondering. We will work to replicate the issue and confirm it's not specific to us and try to document a new issue if so.

joegaudet commented 3 years ago

Thanks @mjy I've been off for the week, so I haven't put any more cycles against this.

keithdoggett commented 3 years ago

@joegaudet if you get a chance to test this out again let me know. If it's working I can make a release with the patch.

joegaudet commented 3 years ago

@keithdoggett sorry about this, I was out for vacation, just confirmed the patch works as expected once I killed spring!

joegaudet commented 3 years ago

Please publish away :)

joegaudet commented 3 years ago

Actually, scratch that, seems to still truncate the tables.

It does fine on the first test run, but on subsequent ones, the tables get emptied.

joegaudet commented 3 years ago

@keithdoggett using the step debugger, it seems your fix doesn't actually get invoked from here:

https://github.com/rails/rails/blob/main/activerecord/lib/active_record/tasks/database_tasks.rb#L261

Which is invoked here:

https://github.com/rails/rails/blob/main/activerecord/lib/active_record/tasks/database_tasks.rb#L412

When I set a breakpoint I still end up in the abstract implementation, which as you can see from a screenshot has the spatial_refs_table:

Screen Shot 2021-06-09 at 6 26 17 PM

joegaudet commented 3 years ago

I can confirm that the module is correctly prepended as well:

Screen Shot 2021-06-09 at 6 31 42 PM

joegaudet commented 3 years ago

@keithdoggett update, it works correctly if I do the following:

ActiveRecord::ConnectionAdapters::PostGISAdapter.prepend(ActiveRecord::ConnectionAdapters::PostGISDatabaseStatements)
keithdoggett commented 3 years ago

Good find! Not sure why that is. I should have some time to take a look at this more in the next few days.

joegaudet commented 3 years ago

No worries! I've got it in my test_helper.rb currently so I'm good for now :)

keithdoggett commented 3 years ago

Hi @joegaudet I opened up a PR #345 that I believe will fix the issue based on your comment. Would you mind building from that branch and testing if it works?

You can install it if you add this to your Gemfile

gem 'activerecord-postgis-adapter', git: 'https://github.com/rgeo/activerecord-postgis-adapter.git', branch: 'parallelize-srs-table'
joegaudet commented 3 years ago

Hey @keithdoggett,

Sorry was slow on this one, been a busy month. Looks like everything works fine, you're free to merge!

.joe

arthurwozniak commented 3 years ago

Hi, is there any chance for new release with this fix in near time?

keithdoggett commented 3 years ago

Hi @arthurwozniak yes we've been a little busy with other work on the core gem, but are in the process of getting this fix reviewed and it should be merged soon.

keithdoggett commented 3 years ago

Released in versions 7.1.1 and 6.0.3