connection_factory ignored

Actually no: the comment in connect() says it was done on purpose: when we added the keyword passthrough we reproduced the same bugs.

I want to clean up the pile of hacks in the connection function anyway (they are documented in psycopg2/__init__.py: I prefer to avoid doing it for 2.4.6 but we could do it for a following version 2.5.

I thought I was following, now I am not sure:) Is it considered a bug that the connection_factory argument is wrong and the connection still can be made?

FYI using 2.4.2 I get:

con=psycopg2.connect(DSN, conection_factory=psycopg2.extras.RealDictConnection)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
/home/aklaver/<ipython-input-4-44ada10fd237> in <module>()
----> 1 con=psycopg2.connect(DSN, conection_factory=psycopg2.extras.RealDictConnection)

TypeError: 'conection_factory' is an invalid keyword argument for this function

Yes, actually it is. I cannot remember exactly in which case the c implementation would have swallowed the keyword arguments, apparently not in all of them. Quoting the comment I put there implementing the python version of connect (i.e. one converting all the keywords into a conninfo string and just handing it to the C connection:

# Note: reproducing the behaviour of the previous C implementation:
# keyword are silently swallowed if a DSN is specified. I would have
# raised an exception. File under "histerical raisins".

Another quirk of the same function is that specifying a negative port it is swallowed as well.

fog, what shall we do: shall we clean up this parameters handling? We have kept them this way for backward compatibility but honestly we have taken the risk order times to improve the library: I think these quirk should just go. After all swallowing the args and adopt the wrong behaviour (such as not going async because somebody has specified (DSN, asinc=True) is not the most helpful thing...

Per my example from 2.4.2, in the past the code did choke on improper arguments. I am all for restoring the previous behavior. When someone explicitly calls for a behavior the program should honor that and fail if the information is wrong.

I propose the attached patch. Is that ok?

I not quite sure what that changes. In both cases you end up with:

return _connect(dsn, connection_factory=connection_factory, async=async)

It would seem that there should be an exception in either case if connection_factory is not correctly specified. Seems to me the problem lies further on in the code where _connect() is processed.

In your case, either you have specified "conection_factory" together with the dns, which is forbidden, or you don't have a dns, in which case "conection_factory" will be passed as dns to PQconnect, that will raise an exception as the parameter is not supported.

Oh, now I remember what the above quirk was about: the behaviour emulated was to silently discard valid keywords if the dns was specified, i.e. in:

connect("dbname=foo", database="bar")

"bar" is swallowed. With my patch it would raise an exception instead.

The code invoked by connect(), i.e. _connect(), doesn't perform any check at all: it passes the conninfo string to the libpq and raises whatever exception the library complains about (different libpq versions support different arguments).

How can it be forbidden? When:
_connect(dsn, connection_factory=connection_factory, async=async)
I see both a dsn and a connection_factory argument.

connection_factory and async are not handled by the libpq: they are handled separately as named parameters and don't end up into kwargs. If you spell them wrongly they end up in the kwargs and passed to the libpq, which will moan.

Please, if you are not convinced, try to apply the patch and see if it works for you. It's pure Python so you don't even need to recompile psycopg. Note that by mistake I've posted a change in setup.cfg that was sitting in my working tree: you can ignore that.

Patched and it works after a fashion. In particular the error message is confusing:
InterfaceError: you cannot specify keyword arguments (conection_factory) together with a connection string

You can specify a keyword argument with a connection string:

con=psycopg2.connect(DSN, connection_factory=psycopg2.extras.RealDictConnection)

The error message from 2.4.2 was clearer about was going on:
'conection_factory' is an invalid keyword argument for this function

The issue seems to be there are two classes of keyword arguments, those that can be passed to Postgres and those that are psycopg2 specific. In the function signature they are presented as equal, but in reality they are not.
The patch forces an exception which is a good thing, it just needs some message clarification to reduce possible confusion.

The problem is that we don't know anymore what keyword argument is allowed and what not: all the keywords (except the two you correctly identify as psycopg-specific) are passed through to the libpq after values escaping.

The message states the fact that "conection_factory" is not allowed as a keyword when DSN is specified: we don't know if it's a good keyword or not. If you try to call the function without the DSN param you will see it fail with a different error (because the keyword has made it to the libpq, which has rejected it).

If you suggest a better wording that would be great, thank you and thank you for testing.

Maybe "'foo' is an invalid keyword argument when the connection string is specified"?.

...or "when a connection string is specified"?

Patch is fine, IMHO. Historically the swallowing was a bug and even if we break compatility on this the user already has a broken DSN (kwargs or not). So fixing this is OK.

Patches are pushed into connect-keywords branch. The final wording used for the error is: "'blah' is an invalid keyword argument when the dsn is specified" (the name of the param is now explicit).

Just to pull out all the skeletons from the closets, the function has historically raised InterfaceError. The standard Python error should be TypeError instead. As I explain in the commit message:

Raise TypeError instead of InterfaceError on bad params on connect()

TypeError is the standard Python error raised in this case:

    $ python -c "(lambda a: None)(b=10)"
    TypeError: <lambda>() got an unexpected keyword argument 'b'

We only used to raise InterfaceError when connect was used without
any parameter at all, so it's hard to think a program depending on
that design. Furthermore the function has always raised (and still
does) OperationalError too, if the bad argument is detected by the
libpq, and that cannot be changed because we can't tell the
difference from a normal connection error.

So with the last commit we change raising InterfaceError + OperationalError (the first only raised by calling a naked connect() to raising TypeError + OperationalError. Is that ok? If not we can avoid merging this commit into devel.

Seems fine to me.

Works for me. Not sure I follow the InterfaceError/TypeError debate though. Per my example from 2.4.2 above, psycopg2 used to raise TypeError for invalid parameters.

My above remark was more directed to the error you get when you call connect(DSN, database='x'), with a keyword together the DSN.

TypeError is definitely the exception to raise for the bad parameters handling you mention in this issue. Python used to do it for us before introducing **kwargs, now we have to do it ourselves.