update_attributes broken w/ postgresql
#9418
Comments
|
Yeah, I've been digging deep into it. I cleaned up the relevant part to: klass = self.class
conn = klass.connection
scope = klass.where(klass.arel_table[klass.primary_key].eq(id))
substitutes = {}
values = {}
attributes_with_values.each_with_index do |tuple, i|
attr, value = tuple
column = klass.columns_hash[attr.name]
substitutes[attr] = conn.substitute_at(column, i)
values[column] = value
end
stmt = scope.arel.compile_update(substitutes)
conn.update stmt, 'SQL', valuesbut it's the same bug. I noticed everywhere else there is a pattern involving Also, inserts are not done with a prepared statement, they're the closest thing I could think of to compare to. Anywhere I can look to see how to do substitutions in general? |
|
also /cc @Noemj who wrote the patch, maybe he can lend a hand? |
|
oh, and the patch also removed a null-value test only for postgresql. Meaning that the patch has removed the support for updated values to being null values! Seems like something we shouldn't do. What was the justification for the substitutions in the first place? Performance? |
|
The select statements work with prepared statements as well; I'm looking into it too. For Googling purposes, the error I got was: |
|
I think the reason was that changes in arel made some fixes necessary (#9246) |
|
More notes: If I look immediately above the error, the other subbed requests are not quoted: So, the sub is being quoted by arel when the update is compiled. And by then it's too late to sub. Also note that in the above it should be |
|
More info: it is the So, what we need is a way to prepare the arel scope into a sql statement with substitutions before sending it to Here's the rewrite I'm working with that makes the actions clearer: klass = self.class
conn = klass.connection
scope = klass.where(klass.arel_table[klass.primary_key].eq(id))
substitutes = {}
values = {}
attributes_with_values.each_with_index do |tuple, i|
attr, value = tuple
column = klass.columns_hash[attr.name]
substitutes[attr] = conn.substitute_at(column, i)
values[column] = value
end
stmt = scope.arel.compile_update(substitutes)
conn.update stmt, 'SQL', values |
|
I found it. It's a bug in Arel that escapes on assignment nodes regardless of if the parameter is a SQL literal or not (which it is). I suspect other database engines don't mind escaping while PG takes offense. I'll prepare a fix. |
|
great. Did this make it into the beta? Also I never checked to see if regular |
|
Looks like it's fixed in the beta. I think that means arel was fixed a while back and maybe we're on old versions of it? It does look like updates are going through as prepared statements. |
|
Yup, I bet it was bd36f18 Closing. |
|
Wow, so much time bug-hunting and it had been fixed the whole time. Ha. |
|
Right, sorry guys for making the arel changes a bit difficult to read on the pull request. I should have emphasized their importance more. Also, postgresql null byte handling was not affected by this update. The null bytes won't throw an error but will truncate the rest of the string, which was also the case before. Null byte support was added for sqlite3 due to changing to prepared statements. The arel changes were indeed necessary for the prepared statements to work, not the other way around. |
|
It's funny, in |
Reproduce
pggem and config database for postgresqlrails g model person name:stringp = Person.create name: "Joe"p.update_attributes name: "Bob"Stack trace:
Related to 45321a6 ?
/cc @tenderlove
xoxo @ngauthier
The text was updated successfully, but these errors were encountered: