Definition of unsafe_add_column is unused

[tragiclifestories]

While trying to test some lock scenarios, I spotted this little problem.

Basically, unlike all the other unsafe_ methods, this one - along with unsafe_make_column_not_nullable - is supposed to have a custom implementation that uses the lock acquisition logic rather than delegating directly to ActiveRecord::Migration. However, the custom implementation are further away in the inheritance chain, so it never gets called - the delegate in unsafe_statements gets called instead.

I thought initially that this was a bug I'd introduced, but I've reproduced it on 1.0.0 as well.

Fix should be as simple as removing the delegate from unsafe_statements ...

EDIT: only unsafe_add_column is affected, not unsafe_make_column_not_nullable as I initially thought.

[tragiclifestories]

Fix should be as simple as removing the delegate from unsafe_statements ...

Actually, it isn't 😞

[jcoleman]

Is there a reason why the lock acquisition shouldn't happen here instead of inside the unsafe_add_column method?

I'm probably missing something obvious...

[tragiclifestories]

From what I understand, the idea is that even if it is impossible to say that the column-add operation itself is safe from an availability point of view, it is still safer to use the lock acquisition loop than not to do so, so unsafe_add_column is still 'safer' for it than vanilla AR.

Cf. the readme:

Unsafely add a column, but do so with a lock that is safely acquired.

[jcoleman]

Huh, I'd forgotten about that part of the README. The question in my mind is "Why is unsafe_add_column special?" After all, I don't think any of the other unsafe variants use safe lock acquisition.

That being said, it might well be the case that all of the unsafe variants (where it's relevant) should use the safe lock acquisition. I think that's mostly a project policy discussion, since it means the unsafe variants are increasingly not just proxies for the original Rails method (though we've already gone down that path somewhat with the dependent object checks).

I think I kinda like continuing down that path. But it will likely end up breaking more compatibility with existing projects. For example, I assume this means locking happens if you use def change to do the auto def down? Since we don't ever use that feature of Rails, I'm not sure what will happen. But it would certainly be surprising if that did break because of this.

I'm curious what your thoughts are on all of that.

[stephenreid]

See also #34

add_column on PostgreSQL > 11 generally shouldn't require this lock; and maybe not even unsafe nomenclature.

[JonMR]

I closed #23 since it's been open for so long, but we think that would make a great starting point.

[jcoleman]

The original bug report here was fixed in #47. I'd been leaving this open because of additional discussion that'd happened here, but I've decided to close this in favor of opening a new issue (#63) to discuss that broader question.