Removes the need for pg client tools #942

jwoertink · 2023-05-15T01:48:27Z

Fixes #926

This doesn't fully remove the need for it, but it does remove the need for it when doing db.create and db.drop tasks.

The two main areas still remaining are db.schema.dump and db.schema.restore tasks which currently require psql and pg_dump to exist. I would like to somehow catch those and raise some sort of error before you run them. Right now if you don't have pg client tools installed and try to run either of those, you'll just end up with some exceptions saying it couldn't find those commands. Not horrible, but not great...

After doing this, there was some weird things that came up. Previously you could do this:

SomeDatabase.run do |db|
  db.exec "SELECT some_sql"
end

DB::Drop.run_task

That seemed to be ok maybe because it was running dropdb from the CLI? 🤷‍♂️ However, now, this raises an exception saying the that DB is currently in use. I guess this is a good thing, but it required adding a way to ensure all connections and the DB were fully closed. I'll add some more comments on this PR

…commands. Ref #926

jwoertink · 2023-05-15T01:49:22Z

spec/avram/migrator/runner_spec.cr

+        Avram.temp_config(database_to_migrate: SampleBackupDatabase) do
+          Avram::Migrator::Runner.create_db(quiet?: true)
+
+          DB.open(SampleBackupDatabase.credentials.url) do |db|


This fully closes the DB connection when the block ends allowing me to drop the DB.

spec/avram/tasks/db_schema_restore_spec.cr

jwoertink · 2023-05-15T01:50:55Z

spec/spec_helper.cr

-class SampleBackupDatabase < Avram::Database
-end
-
-SampleBackupDatabase.configure do |settings|


I moved this to config/database.cr where all the others are. It should have been there in the first place anyway 😂

jwoertink · 2023-05-15T01:53:36Z

src/avram/connection.cr

  def open : DB::Database
-    try_connection!
+    @db = try_connection!
+  end
+
+  def close
+    @db.try(&.close)
+    @db = nil
  end


I don't really understand this. We've never had a db.close before, but it feels like there should have been... I think we just open a connection, then let it sit open in the connection pool forever. If you redeploy your app, that kills all of the connections which may explain why we always get IO errors randomly... But what's a better way to handle this?

Sounds like a plausible explanation. That would solve the bulk of the use cases and the stuff that did generate errors would just get dismissed by restarting the container.

I think this is 👍

jwoertink · 2023-05-15T01:54:22Z

src/avram/credentials.cr

+  def connection_string : String
+    String.build do |io|


In order to run SQL before a database exists, we need the connection string postgres://user:pass@host:5432

What do you mean?

Oh, I was just saying that I moved this logic out so I can differentiate between

postgres://user:pass@host:5432

and

postgres://user:pass@host:5432/some_db

oh I get it. I thought you were saying that you were filling in some sample connection string details for type safety .... o_O I was way off...

src/avram/database.cr

src/avram/migrator/runner.cr

jwoertink · 2023-05-15T01:58:04Z

src/avram/migrator/runner.cr

-      puts "Done creating #{Avram::Migrator::Runner.db_name.colorize(:green)}"
+      puts "Done creating #{db_name.colorize(:green)}"
+    end
+  rescue e : DB::ConnectionRefused


Not sure what level the issue is at, but when you can't connect, a PQ::PQError is raised with a nice helpful error... then that gets swallowed somewhere and a DB::ConnectionRefused is raised in it's place with no error message... so e.message is nil. They just expect you to read through the stack trace.

jwoertink · 2023-05-15T01:59:16Z

src/avram/migrator/runner.cr

+    elsif message.includes?("Cannot establish connection")
      raise PGNotRunningError.new(message)


I actually think this won't ever get here now... I'll have to double check, but I believe this is the same as above. It raises a DB::ConnectionRefused error with no message, and then in the stack trace there's a "Caused by..."

This whole exception logic could use a refactor. For example, matching on exception message text is probably not necessary at all if quiet? is set. And I feel like matching on exception message text isn't a good idea anyway because I wouldn't ever consider that a stable API -- if I were to raise an exception I expect the message to be read by a human.

Totally. This definitely needs a refactor. The main idea here is that the error coming form postgres is generally not too helpful. I find myself usually having to google why I'm getting "could not connect to template1" and other weird errors. So in this case, we catch those, and re-raise a new helpful error that gives you a list of things to look for as to why it failed.

I'll think about this a bit more and circle back.

avram/src/avram/errors.cr

Lines 94 to 99 in 376379a

message << " ▸ Check connection settings in 'config/database.cr'\n"

message << " ▸ Be sure the database exists (lucky db.create)\n"

message << " ▸ Check that you have access to connect to #{connection_details.host} on port #{connection_details.port || DEFAULT_PG_PORT}\n"

if connection_details.password.blank?

message << " ▸ You didn't supply a password, did you mean to?\n"

end

Yeah, there's no reason it needs to be addressed in your pull here, it's out of scope.

jwoertink · 2023-05-15T02:01:00Z

The ameba error will be fixed on the next release (crystal-ameba/ameba#372). But I need to look in to shard edge issue.

jkthorne · 2023-05-15T14:29:57Z

This dependency has caused me problems in the past. I ❤️ this change!

robacarp

This is 👍 . I don't think it's unreasonable to depend on pg_dump and pg_restore -- I think those are more widely available with any postgres tools install. Whereas createdb and friends aren't as widely distributed scripts.

robacarp · 2023-05-15T15:14:42Z

src/avram/connection.cr

  def open : DB::Database
-    try_connection!
+    @db = try_connection!
+  end
+
+  def close
+    @db.try(&.close)
+    @db = nil
  end


Sounds like a plausible explanation. That would solve the bulk of the use cases and the stuff that did generate errors would just get dismissed by restarting the container.

I think this is 👍

robacarp · 2023-05-15T15:15:17Z

src/avram/credentials.cr

+  def connection_string : String
+    String.build do |io|


What do you mean?

src/avram/database.cr

robacarp · 2023-05-15T15:22:37Z

src/avram/migrator/runner.cr

+    elsif message.includes?("Cannot establish connection")
      raise PGNotRunningError.new(message)


This whole exception logic could use a refactor. For example, matching on exception message text is probably not necessary at all if quiet? is set. And I feel like matching on exception message text isn't a good idea anyway because I wouldn't ever consider that a stable API -- if I were to raise an exception I expect the message to be read by a human.

…ome DB is connected when there's no DB by the running username.

jwoertink · 2023-05-20T18:25:39Z

Alright, had to tell create and drop DB that it has to connect to some database... not sure why ONLY that one instance required it, but my guess is that you'll always have a DB named the same as your runner's name. If not, then we'd have to make that configurable as "maintenance_database_name" or something weird...

Removes the need for pg client tools when running create and drop db …

268e036

…commands. Ref #926