feat: commit_comment option for prepending a SQL comment to a COMMIT statement

[bceskavich]

We're using this option at @knocklabs to inject comments for all transactions executed via the Ecto.Repo.transaction/2 callback, which is gives us more granular insight into transaction performance in our RDS performance insights dashboard. We'd like to upstream it!

I wasn't sure where to add unit test coverage for this, if anywhere. Nor was I sure how I should try to sanitize the input, if at all.

More context in the ecto-elixir Google Group discussion here: https://groups.google.com/g/elixir-ecto/c/vUImb5ZX_CI

[josevalim]

Thank you. I have some questions:

1. Should we use -- or /* ... */? Do they work the same?
2. Regardless of the form that we choose, we should probably validate it against SQL injection.
3. We'd also need docs. :)

[bceskavich]

Should we use -- or /* ... */? Do they work the same?

We opted for -- (single-line comment) plus a newline char before the COMMIT because we found it helps to provide visual distinction between the comment and the actual SQL command. Functionally I believe they are identical besides the full-line/multi-line behavior, and I don't have a strong opinion on which should be used.

Here's a real example from our RDS insights dashboard:

Inline query preview	Full query highlight view

Regardless of the form that we choose, we should probably validate it against SQL injection.

Are there any shared tools or preexisting patterns for doing this in the Postgrex codebase that I should reference?

We'd also need docs. :)

Yup of course, will add those in! 😄

[josevalim]

I think we should go with /* then because it will allow multi-line comments. Then, to validate it doesn't inject SQL, we just need to validate it doesn't have "*/" in it.

[bceskavich]

@josevalim I just pushed updates to add docs + validate against SQL injection. For testing the validation flow, should that live in TransactionTest?

[josevalim]

SOunds good yeah

[bceskavich]

Alright @josevalim just pushed up a unit test. I think this is ready for another review.

[dkuku]

Should this be named differently? transaction_comment maybe.

[josevalim]

Let's return Postgrex.Error. It is not worth introducing a new exception type only for this case. WDYT? :)

[bceskavich]

Fine with me!

[josevalim]

We can also return a RuntimeError if you prefer.

[bceskavich]

Went with Postgrex.Error in af319f9

[josevalim]

💚 💙 💜 💛 ❤️