Made the log method a weakreference

[spatankar-dmpna]

This is a fix for issue 1670. The issue outlines a bug where garbage collector cannot destroy the LoggingConnection object since it has a bound method called log on it.

[dvarrazzo]

I played a bit more with the issue. I didn't realise that the pattern of copying a bound method to the state creates a loop. It might be somewhere else too, sure; I have in mind psycopg 3 date/time adapters; I don't know about psycopg2.

However, looking at it, I don' think I would use weakref at all. ISTM that the same can be achieved by taking a class method reference in the state, instead of the bound method. Something like, untested:

        if _logging and isinstance(
                logobj, (_logging.Logger, _logging.LoggerAdapter)):
            self._log_method = type(self)._logtologger
        else:
            self._log_method = type(self)._logtofile

    def log(self, msg, curs):
        """
        Public interface of the log method defined in initialize
        """
        self._log_method(self, msg, curs)

[dvarrazzo]

...or, even cleaner, make _logtologger, _logtofile two staticmethods (taking a self as first parameter anyway) and avoid taking them from type(self) in the constructor.

[dvarrazzo]

See https://github.com/psycopg/psycopg/commit/36b0a5506bb4fec70ca7cb9fa38a88c2e73a094d for an example of the same problem addressed in psycopg 3.

[dvarrazzo]

As for whether there are similar cases in psycopg2, using ag 'self\..* = self\.' I could only see the pattern used in the hstore:

https://github.com/psycopg/psycopg2/blob/00870545b72231775258af4a4dd40842d2f18f0f/lib/extras.py#L797-L798

It's worth fixing it for consistency, however that case only happens with hstore before Postgres 9.0 it seems, so only with long unsupported Postgres versions.

[spatankar-dmpna]

Why is taking self as a parameter for static methods better than using a weak reference for the bound method? Everything I've been looking into tells me not to do that. My PyLint checker throws up a warning when I attempt to do that.

[dvarrazzo]

It is replacing a mechanism fundamentally "magic" with something less magic.

What have you read around about it not being a good idea? It's like saying that writing a function that taking an object as parameter is a bad idea... because that's what it is.

PyLint is full of... bad ideas. However, what's the problem with it? Just that the name of the parameter is "self"? You can call it "obj" if you prefer.