MIME-Version: 1.0
References: 
 <CAC5UznEsaG75-Q89z4Ypz1q48UT7O+1U7drUPt6Xs+ma9_hEGQ@mail.gmail.com>
In-Reply-To: 
 <CAC5UznEsaG75-Q89z4Ypz1q48UT7O+1U7drUPt6Xs+ma9_hEGQ@mail.gmail.com>
From: "Inoue,Hiroshi" <hinoue205@gmail.com>
Date: Tue, 4 Jan 2022 13:21:49 +0900
Message-ID: 
 <CAFGcedX2zgn+iCez7Z3V1G2NLLPimZSP+m_M0BJP3yjXQzJ3=g@mail.gmail.com>
Subject: Re: BUG: CC_send_query_append incomplete error handling
To: Nicolae Vartolomei <nv@cloudflare.com>
Cc: "pgsql-odbc@postgresql.org" <pgsql-odbc@postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000006cf88105d4b9fa62"
Archived-At: 
 <https://www.postgresql.org/message-id/CAFGcedX2zgn%2BiCez7Z3V1G2NLLPimZSP%2Bm_M0BJP3yjXQzJ3%3Dg%40mail.gmail.com>
Precedence: bulk

--0000000000006cf88105d4b9fa62
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Nicolae,

Sorry for the late reply.
I would take care of this issue.

regards,
Hhiroshi Inoue

2022=E5=B9=B41=E6=9C=881=E6=97=A5(=E5=9C=9F) 20:32 Nicolae Vartolomei <nv@c=
loudflare.com>:

> Hi,
>
> I noticed a bug where psqlodbc doesn't handle an underlying error
> correctly.
>
> The interaction is roughly as follows:
>
> Initiate a connection, run few queries, run `ss -K dport postgresql` to
> kill the
> psqlodbc connection to postgres, then run another query.
>
> The expected behaviour is for query to fail and to return an error like
> "08S01".
>
> Instead, the following error is generated:
> ```
> pgapi30.c[PGAPI_GetDiagRec]43: entering type=3D3 rec=3D1
> environ.c[ER_ReturnError]202: entering status =3D 1, msg =3D #no
> connection to the server...
> environ.c[ER_ReturnError]259:      szSqlState =3D 'HY000',len=3D58,
> szError=3D'no connection to the server
> ```
>
> This makes it a bit tricky to properly handle the exception on the client=
.
>
> I'm browsing through this code base for the first time,
> and the incomplete error handling seems to be around these lines in
> connection.c
>
> ```
> if (!PQsendQuery(self->pqconn, query_buf.data))
> {
> char *errmsg =3D PQerrorMessage(self->pqconn);
> QLOG(0, "\nCommunication Error: %s\n", SAFE_STR(errmsg));
> CC_set_error(self, CONNECTION_COMMUNICATION_ERROR, errmsg, func);
> goto cleanup;
> }
> ```
>
> I think this need a call to `CC_on_abort(self, CONN_DEAD);` in order
> to clean up the connection so that future calls would get trapped by
> `SC_connection_lost_check` call
> which also propagates the correct error to `SQLGetDiagRec` with a
> statement handle.
>
> Latest tested version is from this package
> https://packages.ubuntu.com/focal/odbc-postgresql
>
> Let me know if a test case for reproducing the issue is needed. The
> issue affects  this code
>
> https://github.com/ClickHouse/ClickHouse/blob/86040a15d80c9371639ee0fac28=
79da749612a3f/programs/odbc-bridge/ODBCConnectionFactory.h#L76-L96
> (nanodbc-
> <https://github.com/ClickHouse/ClickHouse/blob/86040a15d80c9371639ee0fac2=
879da749612a3f/programs/odbc-bridge/ODBCConnectionFactory.h#L76-L96(nanodbc=
->
> >unixODBC->psqlodbc)
>
> ---
>
> nvartolomei
>
>
>

--0000000000006cf88105d4b9fa62
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hi Nicolae,<div><br></div><div>Sorry for the late reply.</=
div><div>I would take care of this issue.</div><div><br></div><div>regards,=
</div><div>Hhiroshi Inoue</div></div><br><div class=3D"gmail_quote"><div di=
r=3D"ltr" class=3D"gmail_attr">2022=E5=B9=B41=E6=9C=881=E6=97=A5(=E5=9C=9F)=
 20:32 Nicolae Vartolomei &lt;<a href=3D"mailto:nv@cloudflare.com">nv@cloud=
flare.com</a>&gt;:<br></div><blockquote class=3D"gmail_quote" style=3D"marg=
in:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1e=
x">Hi,<br>
<br>
I noticed a bug where psqlodbc doesn&#39;t handle an underlying error corre=
ctly.<br>
<br>
The interaction is roughly as follows:<br>
<br>
Initiate a connection, run few queries, run `ss -K dport postgresql` to kil=
l the<br>
psqlodbc connection to postgres, then run another query.<br>
<br>
The expected behaviour is for query to fail and to return an error like &qu=
ot;08S01&quot;.<br>
<br>
Instead, the following error is generated:<br>
```<br>
pgapi30.c[PGAPI_GetDiagRec]43: entering type=3D3 rec=3D1<br>
environ.c[ER_ReturnError]202: entering status =3D 1, msg =3D #no<br>
connection to the server...<br>
environ.c[ER_ReturnError]259:=C2=A0 =C2=A0 =C2=A0 szSqlState =3D &#39;HY000=
&#39;,len=3D58,<br>
szError=3D&#39;no connection to the server<br>
```<br>
<br>
This makes it a bit tricky to properly handle the exception on the client.<=
br>
<br>
I&#39;m browsing through this code base for the first time,<br>
and the incomplete error handling seems to be around these lines in connect=
ion.c<br>
<br>
```<br>
if (!PQsendQuery(self-&gt;pqconn, query_buf.data))<br>
{<br>
char *errmsg =3D PQerrorMessage(self-&gt;pqconn);<br>
QLOG(0, &quot;\nCommunication Error: %s\n&quot;, SAFE_STR(errmsg));<br>
CC_set_error(self, CONNECTION_COMMUNICATION_ERROR, errmsg, func);<br>
goto cleanup;<br>
}<br>
```<br>
<br>
I think this need a call to `CC_on_abort(self, CONN_DEAD);` in order<br>
to clean up the connection so that future calls would get trapped by<br>
`SC_connection_lost_check` call<br>
which also propagates the correct error to `SQLGetDiagRec` with a<br>
statement handle.<br>
<br>
Latest tested version is from this package<br>
<a href=3D"https://packages.ubuntu.com/focal/odbc-postgresql" rel=3D"norefe=
rrer" target=3D"_blank">https://packages.ubuntu.com/focal/odbc-postgresql</=
a><br>
<br>
Let me know if a test case for reproducing the issue is needed. The<br>
issue affects=C2=A0 this code<br>
<a href=3D"https://github.com/ClickHouse/ClickHouse/blob/86040a15d80c937163=
9ee0fac2879da749612a3f/programs/odbc-bridge/ODBCConnectionFactory.h#L76-L96=
(nanodbc-" rel=3D"noreferrer" target=3D"_blank">https://github.com/ClickHou=
se/ClickHouse/blob/86040a15d80c9371639ee0fac2879da749612a3f/programs/odbc-b=
ridge/ODBCConnectionFactory.h#L76-L96<br>
(nanodbc-</a>&gt;unixODBC-&gt;psqlodbc)<br>
<br>
---<br>
<br>
nvartolomei<br>
<br>
<br>
</blockquote></div>

--0000000000006cf88105d4b9fa62--