Creating index on a column named as a reserved word

[emadshaaban92]

Elixir version

1.16

Database and Version

Postgres 16

Ecto Versions

3.11

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.17.5

Current behavior

Having a column named as a reserved word e.g. offset (my bad)

Creating an index on it using an atom like this works fine

create index("example", [:offset])

But doing the same using a string like this doesn't

create index("example", ["offset"])

It says ** (Postgrex.Error) ERROR 42601 (syntax_error) syntax error at or near "offset"

Expected behavior

I expect it to work, I can use an atom but I faced this issue while using Ash generated migrations.

looking into it, I found that the postgres adapter here properly quotes atoms and doesn't quote string literals (probably to support expressions in indexes)

Postgres docs say that expressions have to have parentheses in them, either the whole expression between parentheses or it's a simple function call which also has parentheses.

The syntax of the CREATE INDEX command normally requires writing parentheses around index expressions, as shown in the second example. The parentheses can be omitted when the expression is just a function call, as in the first example. https://www.postgresql.org/docs/current/indexes-expressional.html

It seems simple to fix, so I'm submitting a PR, I also submitted a PR in ash_postgres but fixing it here still feels more natural, and simpler :)

[josevalim]

Thank you. I believe this is by design. Atoms mean column names, strings mean expressions. Perhaps it should be clearer in the docs or perhaps we should make expressions a bit more opt-in by wrapping in a tuple, but I don’t think we should try to manipulate the expressions.

thoughts @v0idpwn @greg-rychlewski?

[emadshaaban92]

Thanks for the very quick reply Jose (as usual).

Good point, but if I understand correctly making it opt in by wrapping in a tuple wouldn't be backword compatible, right? :thinking:

[greg-rychlewski]

I took a look at the other functions that use columns like add, modify, remove and they all seem to enforce columns names are atoms. So I am in favour of keeping that consistent in index as well.

I'm not really in favour of treating string input differently depending on whether or not there is a parenthesis in it. That seems a bit like a hidden grenade to me.

I would rather add something to the end of the docs like "If the expression is a column name it will not be quoted. This may cause issues when the column is named after a reserved word. Consider using an atom instead"

The fields can be atoms, representing columns, or strings, representing expressions that are sent as-is to the database.

I'm not sure it's worth wrapping in a tuple...this conditions to trigger this seem pretty rare.

[josevalim]

Alright, so I think improving the docs is the way to go indeed. :)

[emadshaaban92]

I like this clear distinction between columns and expressions, sorry I didn't get it before opening this issue :)

Now I believe it should be fixed in Ash migration generator not here. (And I shouldn't be naming columns after reserved words :laughing: )

I've updated the PR with the suggested docs improvement, and will be waiting for the Ash core team to review my other PR there.

Thanks @josevalim and @greg-rychlewski :heart:

[josevalim]

Closing in favor of the PR!