Repository: ansible/ansible
Pull request: Remove dependency to psycopg2 with dump/restore
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
'dump' and 'restore' state only need pg_dump and pg_restore. These tools 
don't use psycopg2 so this change tries to avoid the use of it in these 
cases.
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
This obsoletes #35907 that fixes #35906.

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Bugfix Pull Request

##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
postgresql_db

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

The db_exists test was replaced with an error detection when piping to 
compression program, using a FIFO file. This effectively reverts #39483, 
that was a fix for #39412.


Problem statement:
postgresql_db dump requires psycopg2
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and devel branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
 - Bug Report

##### COMPONENT NAME
<!---
Name of the module, plugin, task or feature
Do not include extra details here, e.g. "vyos_command" not "the network module vyos_command" or the full path
-->
postgresql_db

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
ansible 2.4.2.0
  config file = /home/user/Documents/ansible_backups/ansible.cfg
  configured module search path = [u'/home/user/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.13 (default, Jan 03 2017, 17:41:54) [GCC]
```

##### CONFIGURATION
<!---
If using Ansible 2.4 or above, paste the results of "ansible-config dump --only-changed"
Otherwise, mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->
```
ANSIBLE_PIPELINING(/home/user/Documents/ansible_backups/ansible.cfg) = True
ANSIBLE_SSH_ARGS(/home/user/Documents/ansible_backups/ansible.cfg) = -q -o Compression=yes -o ControlMaster=auto -o ControlPersist=5m -o CheckHostIP=no -o UserKnownHostsFile=/dev/null
ANSIBLE_SSH_CONTROL_PATH(/home/user/Documents/ansible_backups/ansible.cfg) = %(directory)s/ansible-ssh-%%h-%%r
COLOR_VERBOSE(/home/user/Documents/ansible_backups/ansible.cfg) = bright blue
DEFAULT_FORKS(/home/user/Documents/ansible_backups/ansible.cfg) = 50
DEFAULT_HOST_LIST(/home/user/Documents/ansible_backups/ansible.cfg) = [u'/home/user/Documents/ansible_backups/inventory.py']
HOST_KEY_CHECKING(/home/user/Documents/ansible_backups/ansible.cfg) = False
```

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say "N/A" for anything that is not platform-specific.
Also mention the specific version of what you are trying to control,
e.g. if this is a network bug the version of firmware on the network device.
-->
openSuse Tumbleweed

##### SUMMARY
<!--- Explain the problem briefly -->
When using `state=dump` with postgresql_db module, it fails with the following error:
```
fatal: [my_host]: FAILED! => {"changed": false, "msg": "the python psycopg2 module is required"}
```
However, looking at the source, nothing indicates that psycopg2 is required, since only `psql` is invoked for dumping.

`HAS_PSYCOPG2` should only be tested if the `state` is not `dump` or `restore`

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/database/postgresql/postgresql_db.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/database/postgresql/postgresql_db.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @matburt
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
resolved_by_pr #35907 
cc @Andersson007 @amenonsen @andytom @dagwieers @jbscalia @kostiantyn-nemchenko @nerzhul
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
cc @Dorn- @kustodian @sebasmannem
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: Fix YAML syntax

Patch to review:
diff --git a/changelogs/fragments/53323-no-psycopg2-for-dump-and-restore.yaml b/changelogs/fragments/53323-no-psycopg2-for-dump-and-restore.yaml
new file mode 100644
index 00000000000000..c7e35f17073d89
--- /dev/null
+++ b/changelogs/fragments/53323-no-psycopg2-for-dump-and-restore.yaml
@@ -0,0 +1,8 @@
+bugfixes:
+    - States ``dump`` and ``restore`` only need pg_dump and pg_restore. These tools
+      don't use psycopg2 so this change tries to avoid the use of it in these
+      cases. Fixes https://github.com/ansible/ansible/issues/35906
+    - Replace the fix for https://github.com/ansible/ansible/issues/39412
+      made in https://github.com/ansible/ansible/pull/39483 when using a compression
+      program. This now uses a FIFO file to ensure failure detection of pg_dump.
+      The Windows compatibility is completely dropped in this case.
diff --git a/lib/ansible/modules/database/postgresql/postgresql_db.py b/lib/ansible/modules/database/postgresql/postgresql_db.py
index ab69d6e977f168..cd1f9b602c0555 100644
--- a/lib/ansible/modules/database/postgresql/postgresql_db.py
+++ b/lib/ansible/modules/database/postgresql/postgresql_db.py
@@ -83,6 +83,8 @@
     type: str
     default: postgres
     version_added: "2.5"
+notes:
+- State C(dump) and C(restore) don't require I(psycopg2) since version 2.7.9.
 author: "Ansible Core Team"
 extends_documentation_fragment:
 - postgres
@@ -281,7 +283,12 @@ def db_dump(module, target, target_opts="",
         cmd += " {0} ".format(target_opts)
 
     if comp_prog_path:
-        cmd = '{0}|{1} > {2}'.format(cmd, comp_prog_path, pipes.quote(target))
+        # Use a fifo to be notified of an error in pg_dump
+        # Using shell pipe has no way to return the code of the first command
+        # in a portable way.
+        fifo = os.path.join(module.tmpdir, 'pg_fifo')
+        os.mkfifo(fifo)
+        cmd = '{1} <{3} > {2} & {0} >{3}'.format(cmd, comp_prog_path, pipes.quote(target), fifo)
     else:
         cmd = '{0} > {1}'.format(cmd, pipes.quote(target))
 
@@ -397,9 +404,6 @@ def main():
         supports_check_mode=True
     )
 
-    if not HAS_PSYCOPG2:
-        module.fail_json(msg=missing_required_lib('psycopg2'), exception=PSYCOPG2_IMP_ERR)
-
     db = module.params["db"]
     owner = module.params["owner"]
     template = module.params["template"]
@@ -413,6 +417,11 @@ def main():
     maintenance_db = module.params['maintenance_db']
     session_role = module.params["session_role"]
 
+    raw_connection = state in ("dump", "restore")
+
+    if not HAS_PSYCOPG2 and not raw_connection:
+        module.fail_json(msg=missing_required_lib('psycopg2'), exception=PSYCOPG2_IMP_ERR)
+
     # To use defaults values, keyword arguments must be absent, so
     # check which values are empty and don't include in the **kw
     # dictionary
@@ -437,34 +446,35 @@ def main():
         target = "{0}/{1}.sql".format(os.getcwd(), db)
         target = os.path.expanduser(target)
 
-    try:
-        pgutils.ensure_libs(sslrootcert=module.params.get('ssl_rootcert'))
-        db_connection = psycopg2.connect(database=maintenance_db, **kw)
-
-        # Enable autocommit so we can create databases
-        if psycopg2.__version__ >= '2.4.2':
-            db_connection.autocommit = True
-        else:
-            db_connection.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
-        cursor = db_connection.cursor(cursor_factory=psycopg2.extras.DictCursor)
+    if not raw_connection:
+        try:
+            pgutils.ensure_libs(sslrootcert=module.params.get('ssl_rootcert'))
+            db_connection = psycopg2.connect(database=maintenance_db, **kw)
 
-    except pgutils.LibraryError as e:
-        module.fail_json(msg="unable to connect to database: {0}".format(to_native(e)), exception=traceback.format_exc())
+            # Enable autocommit so we can create databases
+            if psycopg2.__version__ >= '2.4.2':
+                db_connection.autocommit = True
+            else:
+                db_connection.set_isolation_level(psycopg2.extensions.ISOLATION_LEVEL_AUTOCOMMIT)
+            cursor = db_connection.cursor(cursor_factory=psycopg2.extras.DictCursor)
 
-    except TypeError as e:
-        if 'sslrootcert' in e.args[0]:
-            module.fail_json(msg='Postgresql server must be at least version 8.4 to support sslrootcert. Exception: {0}'.format(to_native(e)),
-                             exception=traceback.format_exc())
-        module.fail_json(msg="unable to connect to database: %s" % to_native(e), exception=traceback.format_exc())
+        except pgutils.LibraryError as e:
+            module.fail_json(msg="unable to connect to database: {0}".format(to_native(e)), exception=traceback.format_exc())
 
-    except Exception as e:
-        module.fail_json(msg="unable to connect to database: %s" % to_native(e), exception=traceback.format_exc())
+        except TypeError as e:
+            if 'sslrootcert' in e.args[0]:
+                module.fail_json(msg='Postgresql server must be at least version 8.4 to support sslrootcert. Exception: {0}'.format(to_native(e)),
+                                 exception=traceback.format_exc())
+            module.fail_json(msg="unable to connect to database: %s" % to_native(e), exception=traceback.format_exc())
 
-    if session_role:
-        try:
-            cursor.execute('SET ROLE %s' % pg_quote_identifier(session_role, 'role'))
         except Exception as e:
-            module.fail_json(msg="Could not switch role: %s" % to_native(e), exception=traceback.format_exc())
+            module.fail_json(msg="unable to connect to database: %s" % to_native(e), exception=traceback.format_exc())
+
+        if session_role:
+            try:
+                cursor.execute('SET ROLE %s' % pg_quote_identifier(session_role, 'role'))
+            except Exception as e:
+                module.fail_json(msg="Could not switch role: %s" % to_native(e), exception=traceback.format_exc())
 
     try:
         if module.check_mode:
@@ -487,9 +497,6 @@ def main():
                 module.fail_json(msg=to_native(e), exception=traceback.format_exc())
 
         elif state in ("dump", "restore"):
-            if not db_exists(cursor, db) and state == "dump":
-                module.fail_json(
-                    msg="database \"{db}\" does not exist".format(db=db))
             method = state == "dump" and db_dump or db_restore
             try:
                 rc, stdout, stderr, cmd = method(module, target, target_opts, db, **kw)


Code review comments:
diff_hunk: @@ -83,6 +83,8 @@
     type: str
     default: postgres
     version_added: "2.5"
+notes:
+- State C(dump) and C(restore) don't require I(psycopg2) since version 2.7.9.
path: lib/ansible/modules/database/postgresql/postgresql_db.py
text: @user1:
```suggestion
- State C(dump) and C(restore) don't require I(psycopg2) since version 2.8.
```

@user1:
Seems that's all, waiting for your feedback.