postgresql_ping: return conn error message

[Andersson007]

SUMMARY

Relates to #174

@tcraxs @ @klando @MichaelDBA @hunleyd @marcosdiez , needs your opinion on this:

1. Can this ret val be useful? I.e. to be able to use it inside playbooks (now we can see what's wrong only in warnings).
2. If you think this value is useful, should we return it always (i.e. as an empty string when the connection has succeeded) or only when an error occurres?
3. Any other thoughts?

@marcosdiez maybe we could use the related change in connect_to_db to fill up messages in #173

See the integration tests i added to tests/integration/targets/postgresql_ping/tasks/postgresql_ping_initial.yml to see how it can be used.

# Check conn_err_msg return value
- name: Try to connect to non-existent DB
  become_user: "{{ pg_user }}"
  become: yes
  postgresql_ping:
    db: blahblah
    login_user: "{{ pg_user }}"
  register: result

- name: Check conn_err_msg return value
  assert:
    that:
    - result is succeeded
    - result.conn_err_msg is search("database \"blahblah\" does not exist")

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

postgresql_ping

[hunleyd]

Can this ret val be useful? I.e. to be able to use it inside playbooks (now we can see what's wrong only in warnings).

I'm fairly certain we'd make regular use of it at work in our roles/playbooks

If you think this value is useful, should we return it always (i.e. as an empty string when the connection has succeeded) or only when an error occurred?

I'd say always.

[klando]

Well, from a PostgreSQL point of view this ping feature purpose is not to evaluate DB presence and connection.

So here it looks we're translating psycopg2 (the current python driver) behavior as to be able to connect to a DB or not. But I strongly suggest to look at psycopg3 driver to make sure it'll still work the same.

Because when the collection will move to psycopg3 (which I hope will happen sometime in the future), we won't have to maintain this feature which may require additional code because it's not aligned with upstream feature.

[Andersson007]

But I strongly suggest to look at psycopg3 driver to make sure it'll still work the same.

This feature uses the try-except blocks that had been there before. If psycopg3 brings something incompatible in this part, we will have to refactor it anyway.

[klando]

But I strongly suggest to look at psycopg3 driver to make sure it'll still work the same.

This feature uses the try-except blocks that had been there before. If psycopg3 brings something incompatible in this part, we will have to refactor it anyway.

Again, assuming that this module purpose is to evaluate if a DB exists or if we can connect to it is a bad idea, it is not the purpose of the upstream feature, it is explicit in the PostgreSQL documentation.
The mentioned psycopg3 documentation outlines that because psycopg3 brings native support for this feature.

-1 from me for such a change.

[Andersson007]

Okay, let's vote
+0 (I don't mind to add this if the community thinks this is useful)

[MichaelDBA]

@klando,

Well, from a PostgreSQL point of view this ping feature purpose is not to evaluate DB presence and connection.

Why not? Like I said in a comment on #174, we can use PG PING to do multiple things: evaluate a PG cluster that you can connect to (not providing a DBName that will default to postgres db), and/or a specific database within that cluster (specifying a DBName).

[MichaelDBA]

My 2cents on this is to make it fail if we cannot connect to a cluster FOR ANY REASON. This can easily be achieved by changing one line in postgresql_ping.py:
from:
db_connection = connect_to_db(module, conn_params, fail_on_conn=False)
To:
db_connection = connect_to_db(module, conn_params, fail_on_conn=True)

Or add variables to task to see result parm, is_available:

   register: results
   failed_when: results.is_available == false

[Andersson007]

@MichaelDBA so, you're +1 for this ret val?

[MichaelDBA]

+1 for ret val, but even better if it fails on ANY CONNECT ERROR

[marcosdiez]

I agree with @MichaelDBA. Retval is a good idea, but postgres_ping should fail if it does not connect successfully. Also, the dummy variable should theoretically be called _.

[klando]

I am unsure of the goal for the proposed change. Please be careful not to over-engineer what the upstream project is doing:

PQpingParams reports the status of the server. It accepts connection parameters identical to those of PQconnectdbParams, described above. It is not necessary to supply correct user name, password, or database name values to obtain the server status; however, if incorrect values are provided, the server will log a failed connection attempt.

[Andersson007]

I agree with @MichaelDBA. Retval is a good idea, but postgres_ping should fail if it does not connect successfully. Also, the dummy variable should theoretically be called _.

You're right about _ in a general case but naming it as dummy is required in case of Ansible (i don't know why, I like the underscore more and it's more conventiotal), otherwise sanity tests will fail.

About whether it should fail or not, as we've put a way how to make it failing in the doc #176, I think a new bool parameter, fail_on_error or something (to keep backwards compatibility) would be an overkill.

[Andersson007]

Rebased. Green.
So majority is for the change.
If someone takes a look at the code and approve the PR / suggest changes, it would be great. So folks please do it:)

@marcosdiez after that we could create another PR to use the returned error message in the postgresql_info module.

[klando]

@Andersson007 will it work the same with psycopg3 ?

[Andersson007]

@Andersson007 will it work the same with psycopg3 ?

@klando I don't know, I didn't test it. If psycopg3 returns error messages when errors occur, it should work the same. If it does not (though I think it does), we'll have to refactor things unrelated to this PR anyway.

[klando]

@Andersson007 will it work the same with psycopg3 ?

@klando I don't know, I didn't test it. If psycopg3 returns error messages when errors occur, it should work the same. If it does not (though I think it does), we'll have to refactor things unrelated to this PR anyway.

That's why I added the documentation from psycopg3: what has been done with psycopg2 probably won't work the same with psycopg3. Because psycopg3 offers native support to the feature provided by PostgreSQL...

[Andersson007]

@Andersson007 will it work the same with psycopg3 ?

@klando I don't know, I didn't test it. If psycopg3 returns error messages when errors occur, it should work the same. If it does not (though I think it does), we'll have to refactor things unrelated to this PR anyway.

That's why I added the documentation from psycopg3: what has been done with psycopg2 probably won't work the same with psycopg3. Because psycopg3 offers native support to the feature provided by PostgreSQL...

What do you mean when saying native support?
I don't understand why it is a bad idea to return an error message? The PR just makes the function forwarding the error message (if an error occurs) to the caller and then to the conn_err_msg return value, so that users can use it in their playbooks. Or the implementation is bad?

[klando]

@Andersson007 will it work the same with psycopg3 ?

@klando I don't know, I didn't test it. If psycopg3 returns error messages when errors occur, it should work the same. If it does not (though I think it does), we'll have to refactor things unrelated to this PR anyway.

That's why I added the documentation from psycopg3: what has been done with psycopg2 probably won't work the same with psycopg3. Because psycopg3 offers native support to the feature provided by PostgreSQL...

What do you mean when saying native support? I don't understand why it is a bad idea to return an error message? The PR just makes the function forwarding the error message (if an error occurs) to the caller and then to the conn_err_msg return value, so that users can use it in their playbooks. Or the implementation is bad?

psycopg3 test shows how connection and ping are tested: https://github.com/psycopg/psycopg/blob/master/tests/pq/test_pgconn.py

def test_ping(dsn):
    rv = pq.PGconn.ping(dsn.encode())
    assert rv == pq.Ping.OK

    rv = pq.PGconn.ping(b"port=9999")
    assert rv == pq.Ping.NO_RESPONSE

def test_connectdb(dsn):
    conn = pq.PGconn.connect(dsn.encode())
    assert conn.status == pq.ConnStatus.OK, conn.error_message

It's distinct, ping test uses ping() while connection test uses connect(). Probably other interesting examples to read in the same test file to figure how we're trying to mix 2 features in a single one in this community.postgresql_ping module.

[Andersson007]

I believe the next version of the connector will continue to support exception throwing with messages anyway (as this is conventional). We can also use this code change with postgresql_info to report why it's impossible to fetch info from a concrete DB.

[Andersson007]

Could anyone please review the code?

[klando]

I believe the next version of the connector will continue to support exception throwing with messages anyway (as this is conventional). We can also use this code change with postgresql_info to report why it's impossible to fetch info from a concrete DB.

I am unsure I understood which connector you're talking about:

rv = pq.PGconn.ping(dsn.encode() is a connection function.

[Andersson007]

@klando , you asked whether this functionality (introduced by this patch) will work the same with psycopg3

[klando]

@klando , you asked whether this functionality (introduced by this patch) will work the same with psycopg3

and it won't. Or the motivation of postgresql_ping is not to mimic PostgreSQL PG.Ping.
Depends...

Maybe this is the motivation for this special ansible postgresql_ping which I don't have.

[Andersson007]

We have what we have. And we still don't have psycopg3 released. Could you elaborate a bit more on what you're suggesting? Will the introduction of the ret var be harmful? Anyway, I don't think psycopg2 support will be dropped soon after psycopg3 is released and I hope the collection will continue to support it. The community finds this ret var useful.
If someone wants to use PG.Ping later in the module, they can change the behavior based on connector version and document it.

[klando]

psycopg3 has been released: https://www.psycopg.org/articles/2021/10/13/psycopg-30-released/

PostgreSQL provides special functions for ping which are distinct from the usual connection functions. Psycopg2 does not have such distinction, but psycopg3 does. As a consequence, implementation of PQping in postgresql_ping with psycopg2 does use an usual connection function, but it should switch to using the native ping implementation of psycopg3 (i.e. it won't use a connect anymore, and the retval will be from the ping function).

[klando]

to complete: in order to achieve what this patch is doing for postgresql_ping (not the other modules), we will need 2 connections with psycopg3, 1 for connection to DB, 1 for ping.

[Andersson007]

psycopg3 has been released: https://www.psycopg.org/articles/2021/10/13/psycopg-30-released/

Thanks for the information:)

postgresql_ping will anyway use the connect() function when it runs with psycopg2

[Andersson007]

@felixfontein all committed, PTAL

[felixfontein]

Regarding the discussion earlier in this PR:

1. I think this module should fail on errors. It's unfortunately too late to simply change this; this requires a deprecation period since that would be a breaking change. I think it would be good to change this eventually, though.
2. It is a bit unfortunate that this module is called ping, even though the concept of pinging a PostgreSQL server exists and does something else. But that's very similar to the problem with ansible-core's ping module, which isn't a ICMP ping. How about offering an option which allows what kind of ping to do, with default for the current behavior (to keep backwards compatibility)? Then in the future the current default could be deprecated and eventually changed to the other behavior.

Both these points do not affect the PR though. Having a return value with the error message (or at least that indicates that an error happened) is IMO a very good idea (and I hope someone will implement 1 as well soon :) ). I also don't see how continuing on the current approach (connecting to the DB and executing a command) would cause any problems with psycopg3.

[Andersson007]

@felixfontein thank you for the ideas, sound interesting! I'll think them over and will come up with something soon (I hope).

[Andersson007]

@hunleyd @klando @MichaelDBA @marcosdiez thanks for reviewing!