Bad decoding of connection cancel error message

[dvarrazzo]

Hi @dlax

Sorry, something I overlooked in previous reviews:

psycopg/psycopg/psycopg/pq/pq_ctypes.py

Lines 961 to 963 in 3ce9065

	@property
	def error_message(self) -> str:
	return impl.PQcancelErrorMessage(self.pgcancelconn_ptr).decode()

This can fail if someone connects to a server with non-English localization and encoding non-utf8 (situations like psycopg/psycopg2#1442)

This method should return the unchanged message as bytes, and the decoding should happen keeping into account the server encoding. The pattern is similar to what you can see in the connect() generator.

Looking at the connection, one problem we have is that the connection can tell its encoding via parameter_status(), which is then used to decode the error message. I assume that the cancel connection doesn't have such luxury. What we could do instead is to add an encoding attribute to PGcancelConn, defaulting to utf8, and assign it in PGconn.cancel_conn() e.g. with PGcancelConn(rv, encoding=self.parameter_status(b"client_encoding")).

The reason why I would make the decode happen externally is for symmetry with the PGconn and PGresult message attributes, both returning bytes (the result doesn't have an encoding either, so it's taken from the connection too). But the most important thing is that I prefer that we return a message decoded the wrong way rather than we crash on displaying an error and so we clobber another error.

Yet another way to skin this cat is to add an encoding parameter to both PGconn and PGcancelConn, so that we may treat the objects the same pq.error_message() branch

psycopg/psycopg/psycopg/pq/misc.py

Lines 94 to 98 in 3ce9065

	elif hasattr(obj, "error_message"):
	# obj is a PGconn
	if obj.status == OK:
	encoding = pgconn_encoding(obj)
	bmsg = obj.error_message

and we can extend the error_message() input type to accept a PGcancelConn too.

[dlax]

Yes indeed; I'm not so surprised as that's actually something I noticed when first working on the implementation.

By that time, as I was about to implement the very thing you suggest now, I asked about that on -hackers but got replied that this was unnecessary, so I dropped that, but mentioned it in the message of commit 9bbf503 for future reference.

Mentioning this so that you get the whole picture. I can certainly do the changes discussed here if you still think that's the best way forward.

[dvarrazzo]

Hello! Thank you for the reference to the back story!

So yes, it seems that the problem is easy to solve, there will be no different encoding than ascii/utf8 to consider, and actually using the connection encoding would be the wrong thing to do.

I would still think about uniforming the cancel conn to the other conn and the other objects in the pq library: all other string-returning functions and methods return bytes, so I wouldn't make that method different. I will put a MR together myself so that I can familiarise with that object.