From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: new version of PQconnectdb was:(Re: [HACKERS] Determining client_encoding from client locale) |
Date: | 2009-09-28 01:49:53 |
Message-ID: | 603c8f070909271849x78acc7e6o880a95522dade59@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, Sep 23, 2009 at 3:26 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)ec> writes:
>> i extracted the functions to connect that Heikki put on psql in his
>> patch for determining client_encoding from client locale and put it in
>> libpq so i follow the PQconnectdbParams(* params[]) approach.
>
> I got around to looking at the actual patch a bit. I hadn't understood
> why it was so small, but now I do: it's implemented as a wrapper around
> PQconnectdb. That is, it takes the given keywords and values, and
> builds a conninfo string, which PQconnectdb then has to pull apart
> again. This seems, well, dumb. I'll admit that the wasted cycles are
> probably not much compared to all the other costs of establishing a
> connection. But it means that you're still exposed to all the other
> limitations of PQconnectdb, eg any possible bugs or restrictions in the
> quoting/escaping logic. It seems to me that a non-bogus patch for this
> would involve refactoring the code within PQconnectdb so that the
> keywords and values could be plugged in directly without the escaping
> and de-escaping logic. It doesn't look that hard to pull apart
> conninfo_parse into two or three functions that would support this.
>
> Another reason why it needs refactoring is that this way doesn't expose
> all the functionality that ought to be available; in particular not the
> ability to do an async connection while passing the parameters in this
> style. You need analogs to PQconnectStart and PQconnectPoll too.
> (Actually I guess the existing PQconnectPoll would work fine, but you
> definitely need an analog to PQconnectStart.)
Based on this review, it sounds like this patch will need a major
rewrite before it can be seriously reviewed. Given that, I think it
makes sense to postpone this to the next CommitFest, so I am going to
mark it as Returned with Feedback.
...Robert
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2009-09-28 01:58:46 | Re: TODO item: Allow more complex user/database default GUC settings |
Previous Message | Robert Haas | 2009-09-28 01:38:02 | Re: operator exclusion constraints |