-
Notifications
You must be signed in to change notification settings - Fork 158
Autocommit when executing a COPY command #63
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
base: main
Are you sure you want to change the base?
Conversation
5ac47e4
to
64dc8fb
Compare
The linter is failing due to a line being 81 characters long... personally in my project I disable the (without doing stuff like: |
I'd recommend:
|
@@ -301,6 +307,11 @@ def _fetch_redshift_column_attributes(self, column): | |||
return text | |||
|
|||
|
|||
class RedshiftExecutionCntext(PGExecutionContext_psycopg2): | |||
def should_autocommit_text(self, statement): |
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'd get this to call super and only check for 'COPY' if the result is false
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.
That would indeed be a more robust solution, allowing us to automatically stay compatible with future changes to the psycopg2 dialect.
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.
Yeah, that's better... I don't think that the psycopg2 dialect might need to add keywords very often, but it's the cleaner way
I'll see to do that later (before tomorrow? I've been away in the weekend, and now I'm a bit busy again)
Can we get a test and changelog for this? Does this also cause problems with the regular postgres COPY command? |
I'd prefer from sqlalchemy.dialects.postgresql import pyscopg2 as pgdialect |
Sorry for the delay. Anyhow now I added those small fixes. I'd like to add a test, but I wouldn't be able to test it running in Travis (not easily... and also I soon won't have access anymore to the redshift cluster that I was using for testing), sorry I was not aware of the Postgres COPY command, and it feels weird that the default pgdialect doesn't recognize it as a data-changing operation.... maybe this PR should actually be sent to sqlalchemy itself instead? |
https://bitbucket.org/zzzeek/sqlalchemy/pull-requests/68/ It probably won't be merged upstream, zzzeek says
|
Hey all, just read the SQLAlchemy thread as well as this PR. Do you plan to accept this PR here in sqlalchemy-redshift? |
class RedshiftExecutionContext(pgdialect.PGExecutionContext_psycopg2): | ||
def should_autocommit_text(self, statement): | ||
return super(RedshiftExecutionContext, self).should_autocommit_text() \ | ||
or AUTOCOMMIT_REGEXP.match(statement) |
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 generally prefer to use parentheses for line continuations rather than \
:
return (super(RedshiftExecutionContext, self).should_autocommit_text()
or AUTOCOMMIT_REGEXP.match(statement))
I really appreciate that @berdario made an attempt to get this included upstream. Given the proposal was rejected, I'd be happy to see this merged here. @berdario - You mentioned adding a test in https://bitbucket.org/zzzeek/sqlalchemy/pull-requests/68/ ; would you be able to port that here? @graingert - Are you okay with adding this here considering it's not going into sqlalchemy? |
Thanks for following up, @jklukas. Adding
to my instance of CopyCommand works in the meantime. Side question: When running COPY directly from psql or through a JDBC connection, status & rows loaded are sent to stdout. Ex:
I could find nothing in any attributes of the result object when running CopyCommand. Is there a way to capture this output? |
@kerryshireman - I know that psycopg2 makes this available as cursor.rowcount. I suspect sqlalchemy also makes this available, though I don't know what the method is offhand. |
http://docs.sqlalchemy.org/en/latest/core/connections.html#sqlalchemy.engine.ResultProxy.rowcount |
Thanks guys, though I tried accessing that attribute before and it is always returned as -1 directly after the COPY command is executed. |
@kerryshireman that's nasty, that's probably another bug. |
@jklukas |
I feel strongly against hard-wiring autocommit to COPY commands because it looks like we are losing the ability to rollback a transaction that contains a COPY statement. Thoughts? |
I'm having trouble locating specific SQLAlchemy documentation that speaks to this, but I think the |
@jklukas Thanks for the clarification! I was misled by the subject of this PR into thinking that every COPY statement is going to be committed automatically 😛 |
I just ran into this exact issue, glad to see a solution is already in progress. For now we add an explicit commit after COPY commands, however this does leave us with a situation where BEGIN/ROLLBACK (which we make heavy use of during unit testing) will not apply for COPY operations. |
I think lack of a test was the only thing holding this up. @graingert - Would you consider merging this as-is? |
@jklukas tests, squash, rebase, changelog |
This would help with this issue, I think:
http://stackoverflow.com/questions/28271049/redshift-copy-operation-doesnt-work-in-sqlalchemy#
I'm still new with Redshift, and I haven't checked if there are any Redshift-specific commands aside from
COPY
(which is the only one that I've used as for now)Todos