MySQL and Postgres schema: retrieve columns' comments

[Bahanix]

Following this discussion:

• Fetching tables' and columns' comments #2248

Here is a PR to retrieve columns' comments (+ collation + privileges for MySQL).

As @jeremyevans shared, it may be more appropriate to build an extension, similar to @mpalmer's https://github.com/mpalmer/sequel-pg-comment. No hard feeling if the PR is rejected, I mostly pushed it because it fixes my use case, and it could help people who would come across the same issue.

Thank you for your help finding this solution!

Usage

DB.schema(table_name)
# Each column now has a "comment" key

Implementation notes

MySQL vs Postgres adapters:

For other fields, MySQL adapter returns empty string when there is nothing; while Postgres adapter returns nil. I followed this pattern–but it means the behavior isn't the same for the two of them. I don't have an opinion for this, feel free to comment.

Adding vs retrieving comment

This PR is only about retrieving comment, hence hardcoded SQL queries in the tests.

Tests

rake spec_mysql
rake spec_postgres

[jeremyevans]

I'm still on the fence about accepting this. But one of the prerequisites would be that specs are added to test for expected behavior.

[Bahanix]

Thank you for your review!

I've pushed the changes and updated the PR description with implementation notes, especially regarding how MySQL and Postgres adapters don't return the same thing when there is no comment. Do you think this is ok this way, or should I change it?

[jeremyevans]

Thanks for working on this. We should try to make behavior the same on both databases. Using nil instead of the empty string seems like the best choice.

Considering the patch isn't invasive, and some users will find it useful, I'm inclined to accept this. It does invite the slippery slope (people asking for table comments, support for setting comments, support for other databases, etc.), but I think those may still be better handled by external extensions. I looked briefly at Microsoft SQL Server support, and it's not a simple matter like it is on MySQL or PostgreSQL, so I don't think we should support similar behavior for it.

[mpalmer]

You'll definitely get shoved down the slippery slope. It seems somewhat inconsistent to provide in-core support for just reading some kinds of comments on some database engines, while needing an extension to get more comprehensive support. A mysql_comment plugin would seem to be the more consistent approach, to provide access to whatever support MySQL has for such things.

[Bahanix]

Using nil instead of the empty string seems like the best choice.

Change pushed

To give you some context about the need, Sequel is a dependency of Langchainrb which is a dependency of a project I work on. The more info Langchainrb can retrieve from the DB, the better the underlying AI can perform – this is why reading is enough here.

[jeremyevans]

You'll definitely get shoved down the slippery slope. It seems somewhat inconsistent to provide in-core support for just reading some kinds of comments on some database engines, while needing an extension to get more comprehensive support. A mysql_comment plugin would seem to be the more consistent approach, to provide access to whatever support MySQL has for such things.

I agree that it seems somewhat inconsistent. However, if this handles the needs of 80% of users who would like some comment support, considering the limited invasiveness, I think it should be worth it. I would be open to expanding the same type of support by default on other databases, as long as it was of similar invasiveness, so the slippery slope there doesn't bother me too much. In terms of reading other types of comments, and writing comments, that would be a bigger issue, but maybe it isn't a problem to consider that.

[jeremyevans]

Using nil instead of the empty string seems like the best choice.

Change pushed

Looks like you need to update the MySQL adapter spec you added for the change.

[Bahanix]

Woopsy, fixed

[jeremyevans]

Squash merged at b59c0aa