Please review this patch

Please review this patch

am 09.12.2005 04:14:25 von Ludek Finstrle

Hello,

I prepared fix for Bug "Autocommit and Cancel". It's woking patch
(it's used #ifded NOT_USED). The main changes are in using cancelable
PQsendQuery (it could be ok) and in cleaning and correcting
CC_send_query. CC_send_query should be equivalent with old one
except adding some QR_set_aborted. I'm not sure if it's ok.
It passed ok throught few tests.

Please could you take a look at this patch? Comments are welcome.

Thanks

Luf

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Re: Please review this patch

am 09.12.2005 09:31:12 von Dave Page

=20

> -----Original Message-----
> From: pgsql-odbc-owner@postgresql.org=20
> [mailto:pgsql-odbc-owner@postgresql.org] On Behalf Of Ludek Finstrle
> Sent: 09 December 2005 03:14
> To: pgsql-odbc@postgresql.org
> Subject: [ODBC] Please review this patch
>=20
> Hello,
>=20
> I prepared fix for Bug "Autocommit and Cancel". It's woking patch
> (it's used #ifded NOT_USED). The main changes are in using cancelable
> PQsendQuery (it could be ok) and in cleaning and correcting
> CC_send_query. CC_send_query should be equivalent with old one
> except adding some QR_set_aborted. I'm not sure if it's ok.
> It passed ok throught few tests.
>=20
> Please could you take a look at this patch? Comments are welcome.

Think you forgot to attach the patch Luf :-)

Regards, Dave.

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Re: Please review this patch

am 09.12.2005 11:52:16 von Ludek Finstrle

--PEIAKu/WMn1b1Hv9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

> > I prepared fix for Bug "Autocommit and Cancel". It's woking patch
> > (it's used #ifded NOT_USED). The main changes are in using cancelable
> > PQsendQuery (it could be ok) and in cleaning and correcting
> > CC_send_query. CC_send_query should be equivalent with old one
> > except adding some QR_set_aborted. I'm not sure if it's ok.
> > It passed ok throught few tests.
> >
> > Please could you take a look at this patch? Comments are welcome.
>
> Think you forgot to attach the patch Luf :-)

Ops, my fault. Second try ...

Luf

--PEIAKu/WMn1b1Hv9
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="psqlodbc-cancel_abort.diff"

diff -c psqlodbc.orig\connection.c psqlodbc.cancel_abort\connection.c
*** psqlodbc.orig\connection.c Sun Dec 04 22:16:28 2005
--- psqlodbc.cancel_abort\connection.c Wed Dec 07 13:05:05 2005
***************
*** 1423,1443 ****
CC_send_query(ConnectionClass *self, char *query, QueryInfo *qi, UDWORD flag)
{
QResultClass *cmdres = NULL,
! *retres = NULL,
! *res ;
BOOL clear_result_on_abort = ((flag & CLEAR_RESULT_ON_ABORT) != 0),
! create_keyset = ((flag & CREATE_KEYSET) != 0),
! issue_begin = ((flag & GO_INTO_TRANSACTION) != 0 && !CC_is_in_trans(self));
! char *wq;
! int id=0;
PGconn *pgconn = self->pgconn;
! int maxlen,
! empty_reqs;
! BOOL ReadyToReturn = FALSE,
! query_completed = FALSE,
! before_64 = PG_VERSION_LT(self, 6.4),
! aborted = FALSE,
! used_passed_result_object = FALSE;
int func_cs_count = 0;


--- 1423,1436 ----
CC_send_query(ConnectionClass *self, char *query, QueryInfo *qi, UDWORD flag)
{
QResultClass *cmdres = NULL,
! *retres = NULL,
! *res ;
BOOL clear_result_on_abort = ((flag & CLEAR_RESULT_ON_ABORT) != 0),
! create_keyset = ((flag & CREATE_KEYSET) != 0),
! issue_begin = ((flag & GO_INTO_TRANSACTION) != 0 && !CC_is_in_trans(self));
PGconn *pgconn = self->pgconn;
! int maxlen;
! BOOL used_passed_result_object = FALSE;
int func_cs_count = 0;


***************
*** 1467,1483 ****
if (issue_begin)
{
res = LIBPQ_execute_query(self,"BEGIN");
! QR_Destructor(res);
}

res = LIBPQ_execute_query(self,query);

! if((!res) || (res->status == PGRES_EMPTY_QUERY) )
{
QR_Destructor(res);
res = NULL;
goto cleanup;
}
else if (res->status == PGRES_COMMAND_OK)
{
mylog("send_query: done sending command\n");
--- 1460,1492 ----
if (issue_begin)
{
res = LIBPQ_execute_query(self,"BEGIN");
! if ((!res) || (res->status != PGRES_COMMAND_OK))
! {
! CC_set_error(self, CONNECTION_COULD_NOT_SEND, "Could not send Query to backend");
! CC_on_abort(self, NO_TRANS | CONN_DEAD);
! QR_Destructor(res);
! res = NULL;
! goto cleanup;
! }
}

res = LIBPQ_execute_query(self,query);

! if(!res)
! {
! goto cleanup;
! }
! #ifdef NOT_USED
! else if ((res->status == PGRES_EMPTY_QUERY) || (res->status == PGRES_FATAL_ERROR) ||
! (res->status == PGRES_BAD_RESPONSE))
{
+ mylog("send_query: failed -> abort\n");
+ QR_set_aborted(res, TRUE);
QR_Destructor(res);
res = NULL;
goto cleanup;
}
+ #endif /* NOT_USED */
else if (res->status == PGRES_COMMAND_OK)
{
mylog("send_query: done sending command\n");
***************
*** 1485,1575 ****
}
else
{
! mylog("send_query: done sending query\n");

- empty_reqs = 0;
- for (wq = query; isspace((UCHAR) *wq); wq++)
- ;
- if (*wq == '\0')
- empty_reqs = 1;
cmdres = qi ? qi->result_in : NULL;
if (cmdres)
used_passed_result_object = TRUE;
if (!used_passed_result_object)
{
if (create_keyset)
QR_set_haskeyset(res->next);
if (!QR_fetch_tuples(res, self, qi ? qi->cursor : NULL))
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, QR_get_message(res));
- if (PGRES_FATAL_ERROR == QR_get_status(res))
- retres = cmdres;
- else
- retres = NULL;
}
}
else
! { /* next fetch, so reuse an existing result */
! /*
! * called from QR_next_tuple and must return
! * immediately.
! */
if (!QR_fetch_tuples(res, NULL, NULL))
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, QR_get_message(res));
! retres = NULL;
}
- retres = cmdres;
}
}

cleanup:
CLEANUP_FUNC_CONN_CS(func_cs_count, self);
! /*
! * Cleanup garbage results before returning.
! */
! if (cmdres && retres != cmdres && !used_passed_result_object)
! {
! QR_Destructor(cmdres);
! }
/*
* Cleanup the aborted result if specified
*/
if (retres)
{
! if (aborted)
{
! if (clear_result_on_abort)
! {
! if (!used_passed_result_object)
! {
! QR_Destructor(retres);
! retres = NULL;
! }
! }
! if (retres)
! {
! /*
! * discard results other than errors.
! */
! QResultClass *qres;
! for (qres = retres; qres->next; qres = retres)
! {
! if (QR_get_aborted(qres))
! break;
! retres = qres->next;
! qres->next = NULL;
! QR_Destructor(qres);
! }
! /*
! * If error message isn't set
! */
! if (retres && (!CC_get_errormsg(self) || !CC_get_errormsg(self)[0]))
! CC_set_errormsg(self, QR_get_message(retres));
! }
}
}
- #undef return
return res;
}

--- 1494,1564 ----
}
else
{
! mylog("send_query: done sending query with status: %i\n",res->status);

cmdres = qi ? qi->result_in : NULL;
if (cmdres)
used_passed_result_object = TRUE;
if (!used_passed_result_object)
{
+ if ((res->status == PGRES_EMPTY_QUERY) || (res->status == PGRES_FATAL_ERROR) ||
+ (res->status == PGRES_BAD_RESPONSE))
+ {
+ mylog("send_query: sended query failed -> abort\n");
+ QR_set_aborted(res, TRUE);
+ QR_Destructor(res);
+ res = NULL;
+ goto cleanup;
+ }
if (create_keyset)
QR_set_haskeyset(res->next);
if (!QR_fetch_tuples(res, self, qi ? qi->cursor : NULL))
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, QR_get_message(res));
}
}
else
! {
! /* next fetch, so reuse an existing result */
! mylog("send_query: next fetch -> reuse result\n");
if (!QR_fetch_tuples(res, NULL, NULL))
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, QR_get_message(res));
! if (PGRES_FATAL_ERROR == QR_get_status(res))
! {
! QR_set_aborted(res, TRUE);
! retres = cmdres;
! }
}
}
}

cleanup:
CLEANUP_FUNC_CONN_CS(func_cs_count, self);
! #undef return
/*
* Cleanup the aborted result if specified
*/
if (retres)
{
! /*
! * discard results other than errors.
! */
! QResultClass *qres;
! for (qres = retres; qres->next; qres = retres)
{
! if (QR_get_aborted(qres))
! break;
! retres = qres->next;
! qres->next = NULL;
! QR_Destructor(qres);
}
+ /*
+ * If error message isn't set
+ */
+ if (retres && (!CC_get_errormsg(self) || !CC_get_errormsg(self)[0]))
+ CC_set_errormsg(self, QR_get_message(retres));
}
return res;
}

***************
*** 1782,1788 ****
LIBPQ_execute_query(ConnectionClass *self,char *query)
{
QResultClass *qres;
! PGresult *pgres;
char errbuffer[ERROR_MSG_LENGTH + 1];
int pos=0;

--- 1771,1777 ----
LIBPQ_execute_query(ConnectionClass *self,char *query)
{
QResultClass *qres;
! PGresult *pgres = NULL, *pgresnew = NULL;
char errbuffer[ERROR_MSG_LENGTH + 1];
int pos=0;

***************
*** 1791,1803 ****
if(!qres)
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, "Could not allocate memory for result set");
QR_Destructor(qres);
! return NULL;
}

! PQsetNoticeProcessor(self->pgconn, CC_handle_notice, qres);
! pgres = PQexec(self->pgconn,query);
! PQsetNoticeProcessor(self->pgconn, CC_handle_notice, NULL);

mylog("LIBPQ_execute_query: query = %s\n",query);

--- 1780,1806 ----
if(!qres)
{
CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, "Could not allocate memory for result set");
+ CC_on_abort(self, CONN_DEAD);
QR_Destructor(qres);
! return NULL;
}

! PQsetNoticeProcessor(self->pgconn, CC_handle_notice, qres);
! if (!PQsendQuery(self->pgconn,query))
! {
! CC_set_error(self, CONNECTION_COULD_NOT_RECEIVE, "Could not send query to backend");
! CC_on_abort(self, CONN_DEAD);
! QR_Destructor(qres);
! return NULL;
! }
! while (pgresnew = PQgetResult(self->pgconn))
! {
! mylog("LIBPQ_execute_query: get next result with status = %i\n",PQresultStatus(pgresnew));
! if (pgres)
! PQclear(pgres);
! pgres = pgresnew;
! }
! PQsetNoticeProcessor(self->pgconn, CC_handle_notice, NULL);

mylog("LIBPQ_execute_query: query = %s\n",query);


--PEIAKu/WMn1b1Hv9
Content-Type: text/plain
Content-Disposition: inline
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

--PEIAKu/WMn1b1Hv9--

Re: Please review this patch

am 10.12.2005 00:36:28 von Dave Page

=20

> -----Original Message-----
> From: Ludek Finstrle [mailto:luf@pzkagis.cz]=20
> Sent: 09 December 2005 10:52
> To: Dave Page
> Cc: pgsql-odbc@postgresql.org
> Subject: Re: [ODBC] Please review this patch
>=20
> > > I prepared fix for Bug "Autocommit and Cancel". It's=20
> woking patch
> > > (it's used #ifded NOT_USED). The main changes are in=20
> using cancelable
> > > PQsendQuery (it could be ok) and in cleaning and correcting
> > > CC_send_query. CC_send_query should be equivalent with old one
> > > except adding some QR_set_aborted. I'm not sure if it's ok.
> > > It passed ok throught few tests.
> > >=20
> > > Please could you take a look at this patch? Comments are welcome.
> >=20
> > Think you forgot to attach the patch Luf :-)
>=20
> Ops, my fault. Second try ...

OK, I've bashed it around a fair bit and can't find any regressions over
the previous code, so I'll commit the code. I don't have any threaded
test code to properly test SQLCancel with at the moment though, so
perhaps the original reporter can test that.

Regards, Dave.

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Re: Please review this patch

am 10.12.2005 06:55:33 von Ludek Finstrle

> OK, I've bashed it around a fair bit and can't find any regressions over
> the previous code, so I'll commit the code. I don't have any threaded
> test code to properly test SQLCancel with at the moment though, so
> perhaps the original reporter can test that.

Ok, I'll include it in next development snapshot.
Thare is a lot of problems with SQLCancel. It needs huge changes.
I think I clean the code at first. There is a lot of dead ends.

Thanks,

Luf

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly