-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Postgresql CSV import for older versions. #487
base: master
Are you sure you want to change the base?
Conversation
Postresql's COPY command syntax has changed from 8.x to 9.x. odo uses the new syntax, which produces a syntax error when running against 8.x and older. Below is a patch that probes the database version and applies the correct syntax. It also fixes a (suspected) bug in how encoding is inferred.
22b4c6f
to
8f1bf9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it has taken so long to get to this. We recently made a change to use COPY FROM STDIN
for a big performance win. Do you think you could rebase against those changes and possibly add them to the pg 8 branch if it makes sense?
), | ||
**kwargs | ||
) | ||
postgres_version = element.bind.execute('select version()').scalar() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried about the perf penalty we will pay to check this on every insert. What do you think about creating a WeakKeyDictionary
from bind
to version? This will let us save these checks across calls.
escapechar=element.escapechar, | ||
header=element.header, | ||
encoding=element.encoding or element.bind.execute( | ||
'show client_encoding' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably also memoize the encoding if we are caching the version.
) | ||
postgres_version = element.bind.execute('select version()').scalar() | ||
if int(postgres_version.split()[1].split('.')[0]) >= 9: | ||
return compiler.process( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in order to make this easier to follow, what do you think about breaking these two calls into helper functions like _process_pg_8_or_lower
or process_pg_9_and_greater
?
Postresql's COPY command syntax has changed from 8.x to 9.x. odo uses
the new syntax, which produces a syntax error when running against 8.x
and older. Below is a patch that probes the database version and applies
the correct syntax. It also fixes a (suspected) bug in how encoding is
inferred.