Adds notifies processing during commit

[romank0]

This fixes the issue that asynchronous notifications are not fetched during the commit #1727.

[romank0]

By some reason the test fails in CI 😕 It might be some difference in notifications delivery in a different version of postgres.

[romank0]

I've checked this locally on the version of postgres 9.6 (the same that is used in CI) and the test works. So this seems to be not the reason of the failure in CI.

[dvarrazzo]

Hello,

your change makes sense.

I wonder if we shouldn't maintain the symmetry and add the conn_notifies_process() call after ROLLBACK too. From a semantic point of view it should do nothing because notifications generated in a transaction are not sent in case of transaction rollback, but from a protocol point of view some notification might be in that message.

About the test failing, I wonder is if there isn't any race condition that gets triggered when the server is remote. Where do you see the failing test?

[romank0]

From a semantic point of view it should do nothing because notifications generated in a transaction are not sent in case of transaction rollback,

The notifications that are generated in the current transaction will not be delivered by postgres but notifications generated by other committed transaction may be delivered in any interaction with the server including the rollback.

I think I can add the similar processing for the rollback that will by no means be the complete fix. This fix is very limited in its scope namely this fixes only the commit.
The proper fix I belive should follow the guidelines from pqlib:

You should, however, remember to check PQnotifies after each PQgetResult or PQexec, to see if any notifications came in during the processing of the command

This means that the notifies checking should happen at every call to these two functions. I'm not however comfortable with doing such massive change without proper understanding how the code works w.r.t. locking and C<->python interaction. Specifically I don't understand if it possible to move the notifies checking into pq_execute_command_locked which is one of the places where PQexec is called.

Where do you see the failing test

On this check there are multiple steps. Some of them fail due to some configuration reason as in test are not run at all:

Running Install scripts
%PYEXE% scripts\build\appveyor.py install
The system cannot find the path specified.
Command exited with code 1

But some steps fail because of the new test that I've added:

======================================================================
FAIL: test_notifies_received_on_commit (tests.test_notify.NotifiesTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Project\tests\testutils.py", line 459, in skip_if_crdb__
    return f(self, *args, **kwargs)
  File "C:\Project\tests\testutils.py", line 529, in slow_
    return f(self)
  File "C:\Project\tests\test_notify.py", line 137, in test_notifies_received_on_commit
    self.assertEqual(1, len(self.conn.notifies))
AssertionError: 1 != 0
----------------------------------------------------------------------
Ran 743 tests in 161.149s
FAILED (failures=1, skipped=33)
Traceback (most recent call last):
  File "scripts\build\appveyor.py", line 846, in <module>
    sys.exit(main())
  File "scripts\build\appveyor.py", line 39, in main
    cmd()
  File "scripts\build\appveyor.py", line 386, in step_test_script
    run_test_suite()
  File "scripts\build\appveyor.py", line 422, in run_test_suite
    run_python(args)
  File "scripts\build\appveyor.py", line 609, in run_python
    return run_command([opt.py_exe] + args, **kwargs)
  File "scripts\build\appveyor.py", line 593, in run_command
    sp.check_call(cmdline, **kwargs)
  File "C:\Python36\lib\subprocess.py", line 311, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['C:\\Python38\\python.exe', '-c', "import tests; tests.unittest.main(defaultTest='tests.test_suite')", '--verbose']' returned non-zero exit status 1.

[dvarrazzo]

Ah, on windows, I see. That's strange: this doesn't seem a platform-dependent change. I would be tempted to just skip the test on window. Of course the tests whose configuration fail are not your fault: the appveyor pipeline hasn't been exercised in a while probably.

[romank0]

If tests are run with debug info this is passed to the subprocess and the output of the script which is used by tests cannot be parsed as it is polluted by the debug output.

[romank0]

I've added notifications processing to all places where PQexec is invoked and disabled tests on windows. I would like to parametrize tests to avoid repetitive code but I am not sure about bringing additional dependencies.

[dvarrazzo]

We massaged a bit Appveyor in order to make it build Python 13 package and as a result tests now pass. I think we can merge this. Thank you very much!