Re: Let's use libpq for everything

On 12/10/2014 07:57 AM, Michael Paquier wrote:
> On Wed, Dec 10, 2014 at 1:20 AM, Heikki Linnakangas <hlinnakangas@vmware.com>
> wrote:
>>> 6) I am getting many regression failures after applying this patch and
>>> running the tests on OSX, please see attached.
>>
>>
>> Attached is a new version [1], with a lot of small fixes here and there.
> It
>> passes all the regression tests for me. Can you try again with this
> version?
>> If it's still failing on OS X, will need to investigate.
>
> With this version regression tests are passing on all my OSX/Linux dev
> machines. At least I do not see failures directly related to it.

Great!

> Few comments:
> 1) Here an error message would be nice:
> +               /*
> +                * XXX: we don't check the result here. Should we? We're
> rolling back,
> +                * so it's not clear what else we can do on error. Giving
> an error
> +                * message to the application would be nice though.
> +                */
> +               if (pgres != NULL)
> +               {
> +                       PQclear(pgres);
> +                       pgres = NULL;
> +               }

Yeah. I left it as it is for now, with the XXX comment. (the
corresponding code in master also ignores errors, so this isn't a
regression)

> 2) Is this planned to be updated after this patch?
> +
> +       /*
> +        * Update our copy of the transaction status.
> +        *
> +        * XXX: Once we stop using the socket directly, and do everything
> with
> +        * libpq, we can get rid of the transaction_status field altogether
> +        * and always ask libpq for it.
> +        */
> +       LIBPQ_update_transaction_status(self);

That idea mentioned in the above comment make sense, but I'm not going
to immediately follow up on it. Besides updating the in-trans and
in-error-trans flags in the connection object,
LIBPQ_update_transaction_status also calls CC_on_abort and CC_on_commit
functions when the state changes, so we can't just directly remove
LIBPQ_update_transaction_status.

> 3) Did you do some performance tests here?
> +       /*
> +        * XXX: We need to do Prepare + Describe as two different
> round-trips
> +        * to the server, while without libpq we send a Parse and Describe
> +        * message followed by a single Sync.
> +        */

Not specifically, but I did analyze the number of round-trips performed
by the regression suite earlier.

I've merged this with latest changes in the master branch, and did some
error handling fixes. Latest version is again attached here, and also
available in github
(https://github.com/hlinnaka/psqlodbc/tree/libpq-integration4).

- Heikki