Add safe_make_column_not_nullable

[alexspeller]

Addresses #106

I also added some quoting for some related methods where it seemed to be missing

[jcoleman]

I'm wondering if we want to also (whether it's in this PR or not) add a method that converts a validated check constraint to a regular NOT NULL constraint (the benefit basically being that it would assert that the constraint exists and is already validated).

[jcoleman]

Are constraint names global? If so, do we want to add a random tag on the end of this?

[rkrage]

Yeah, I think constraint names need to be unique within a schema. Because this method is doing multiple things non-transactionally, there is a possibility we could error out before the constraint is removed. So a) I agree with @jcoleman that we should add some random characters to this string and b) we should document that this might leave behind a constraint on failure, similar to creating a partitioned index.

[alexspeller]

The documentation says that constraints are only unique per-table:

https://www.postgresql.org/docs/current/sql-set-constraints.html#:~:text=Because%20PostgreSQL%20does%20not%20require,will%20act%20on%20all%20matches.

I don't have a strong opinion on the naming of the constraint or if it should still have randomness regardless

[jcoleman]

I wonder if we should add an assertion that this doesn't use locks beyond a certain level.

@rkrage Thoughts on this? I don't think we have this, but I'd love to have a test helper something like this:

expect do
  ...
end.not_to acquire_locks_on_table(...).higher_than(...)

Or similar.

Thinking out loud here:

I don't think there's a way to get Postgres to log all locks acquired, and so we'd probably have to do something like lock the table and then run the second query to verify what type is waiting in pg_locks. That doesn't work easily for multiple statements, though, so I think in this case we might lock the table at a lower level and expect this to proceed. But I think the ALTER TABLE will still need an AccessExclusiveLock, it will just be very short lived.

Edit: apparently log_lock_waits is a thing.

[rkrage]

I'll have to think about this a bit, but I think it's going to be super difficult to make a generic helper. As you pointed out, we could acquire an AccessExclusiveLock and sleep for x seconds in a thread and then concurrently run the code under test. That code would block until the sleep expires and then we could go inspect the logs for the output from log_lock_waits. However, that breaks down for methods that execute multiple SQL statements.

I guess we could do something crazy and mock the connection#execute method with and_wrap_original such that it automatically does the AEL shenanigans described above before any SQL is executed

[rkrage]

Since we don't currently have a precedent for writing such a test, I'm not sure if it's fair to put this on @alexspeller 😄

This can maybe be something we do as a follow on to this PR

[alexspeller]

Yeah realistically I don't have the bandwidth at the moment to deep dive on that unfortunately - I didn't see any precedent for more in-depth lock testing for similar methods so I kept it quite basic