{"action":"created","issue":{"url":"https://api.github.com/repos/apache/incubator-airflow/issues/1821","repository_url":"https://api.github.com/repos/apache/incubator-airflow","labels_url":"https://api.github.com/repos/apache/incubator-airflow/issues/1821/labels{/name}","comments_url":"https://api.github.com/repos/apache/incubator-airflow/issues/1821/comments","events_url":"https://api.github.com/repos/apache/incubator-airflow/issues/1821/events","html_url":"https://github.com/apache/incubator-airflow/pull/1821","id":181134608,"number":1821,"title":"[AIRFLOW-139] Let psycopg2 handle autocommit for PostgresHook","user":{"login":"danielzohar","id":6514918,"avatar_url":"https://avatars.githubusercontent.com/u/6514918?v=3","gravatar_id":"","url":"https://api.github.com/users/danielzohar","html_url":"https://github.com/danielzohar","followers_url":"https://api.github.com/users/danielzohar/followers","following_url":"https://api.github.com/users/danielzohar/following{/other_user}","gists_url":"https://api.github.com/users/danielzohar/gists{/gist_id}","starred_url":"https://api.github.com/users/danielzohar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danielzohar/subscriptions","organizations_url":"https://api.github.com/users/danielzohar/orgs","repos_url":"https://api.github.com/users/danielzohar/repos","events_url":"https://api.github.com/users/danielzohar/events{/privacy}","received_events_url":"https://api.github.com/users/danielzohar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":12,"created_at":"2016-10-05T12:04:32Z","updated_at":"2016-11-22T03:36:48Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/apache/incubator-airflow/pulls/1821","html_url":"https://github.com/apache/incubator-airflow/pull/1821","diff_url":"https://github.com/apache/incubator-airflow/pull/1821.diff","patch_url":"https://github.com/apache/incubator-airflow/pull/1821.patch"},"body":"Dear Airflow Maintainers,\n\nPlease accept this PR that addresses the following issues:\nhttps://issues.apache.org/jira/browse/AIRFLOW-139\n\nRe-posting from the JIRA comment:\n\nThis indeed causes problems with (for example) Redshift. \nI agree special cases should be discouraged though the current implementation definitely feels like a special case handling. \nThere was no issue attached to the original commit (https://github.com/apache/incubator-airflow/commit/28da05d860147b5e0df37d998f437af6a5d4d178) but I'm supposing it came due to https://www.postgresql.org/docs/7.4/static/release-7-4.html.\nThe documentation states:\n\"The server-side autocommit setting was removed and reimplemented in client applications and languages. Server-side autocommit was causing too many problems with languages and applications that wanted to control their own autocommit behavior, so autocommit was removed from the server and added to individual client APIs as appropriate.\"\nAs far as I can see, psycopg2 supports setting autocommit and I'd be very surprised if it didn't handle it well.\nI tested it locally and I can confirm it works well with 9.5.4 with the following code:\n\n```\n>>> import psycopg2\n>>> conn_string = \"host='127.0.0.1' dbname='db' user='user' password='pwd'\"\n>>> conn = psycopg2.connect(conn_string)\n>>> conn.autocommit = True\n>>> cursor = conn.cursor()\n>>> cursor.execute('SELECT VERSION();')\n>>> cursor.fetchall()\n[('PostgreSQL 9.5.4 on x86_64-pc-linux-gnu, compiled by gcc (Debian 4.9.2-10) 4.9.2, 64-bit',)]\n```\n\nIn all honesty, I'm not sure what was the purpose of the original fix and I think those lines should be removed.\n\nWith regards to tests, I can't find where tests for hooks exist at the moment and what kind of things we might want to test. Also, are there any best practices on how we test against databases if at all? I think the added code should have had unit-tests, not really sure what we might want to test by removing the code, that `Postgres.Hook.autocommit = True`?\n"},"comment":{"url":"https://api.github.com/repos/apache/incubator-airflow/issues/comments/262139110","html_url":"https://github.com/apache/incubator-airflow/pull/1821#issuecomment-262139110","issue_url":"https://api.github.com/repos/apache/incubator-airflow/issues/1821","id":262139110,"user":{"login":"r39132","id":581734,"avatar_url":"https://avatars.githubusercontent.com/u/581734?v=3","gravatar_id":"","url":"https://api.github.com/users/r39132","html_url":"https://github.com/r39132","followers_url":"https://api.github.com/users/r39132/followers","following_url":"https://api.github.com/users/r39132/following{/other_user}","gists_url":"https://api.github.com/users/r39132/gists{/gist_id}","starred_url":"https://api.github.com/users/r39132/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/r39132/subscriptions","organizations_url":"https://api.github.com/users/r39132/orgs","repos_url":"https://api.github.com/users/r39132/repos","events_url":"https://api.github.com/users/r39132/events{/privacy}","received_events_url":"https://api.github.com/users/r39132/received_events","type":"User","site_admin":false},"created_at":"2016-11-22T03:36:48Z","updated_at":"2016-11-22T03:36:48Z","body":"Very simple test dag that:\r\n* creates a table\r\n* truncates the table\r\n* inserts records\r\n* calls vacuum analyze\r\n\r\n\r\n<img width=\"1174\" alt=\"screenshot 2016-11-21 19 34 45\" src=\"https://cloud.githubusercontent.com/assets/581734/20509966/9f092f32-b021-11e6-9301-9c0e9cfe94c4.png\">\r\n\r\n<img width=\"1159\" alt=\"screenshot 2016-11-21 19 33 57\" src=\"https://cloud.githubusercontent.com/assets/581734/20509953/825d6e7a-b021-11e6-9adb-1619413904d6.png\">\r\n\r\n<img width=\"576\" alt=\"screenshot 2016-11-21 19 34 59\" src=\"https://cloud.githubusercontent.com/assets/581734/20509964/9d033c5a-b021-11e6-9fda-c7eb7d6d324d.png\">\r\n\r\n"}}
{"actor":{"display_login":"r39132"}}