Re: [Libdbi-drivers-devel] Wrong seeking in dbd_mysql

[Jan E. <je...@in...>]

On Friday 2014-05-02 00:55, Jan Engelhardt wrote:
>On Friday 2014-05-02 00:15, Markus Hoenicka wrote:
>>Am 2014-05-01 18:50, schrieb Jan Engelhardt:
>>> I have identified another bug and prepared a patch for
>>> dbd_mysql/dbd_msql. The purpose of result->currowidx was not totally
>>> clear-cut, so I am posting this patch for potential comments.
>>
>>[...] was designed to avoid an expensive call to mysql_data_seek()
>>in *every single* iteration of stepping through a result set. There
>>are data in the mailing list archives which show that MySQL is
>>slowed down abysmally [...]. It would be great if we could find a
>>way to synchronize result->currowidx with the DBD row pointer in a
>>cheaper way.

Indeed, I found that libmysql uses a singly-linked list. That is truly
abysmal, already from a design point.
I have this new patch, for libdbi this time. Looks better?

====
dbi: resolve bogus seeking into dbd

I have a query result that, when iteratively seeked with
dbi_result_next_row() and read with dbi_result_get_string(), yields
these 15 values (1-based row indices shown in parentheses):

	B-W(1) BAY(2) BER(3) BRE(4) HAM(5) HES(6) M-V(7) NDS(8)
	NRW(9) RLP(10) SAR(11) SAC(12) S-A(13) S-H(14) THU(15)

Now, when using dbi_result_seek_row() to seek to arbitrary rows
(given in parentheses), dbi_result_get_string() can return incorrect
values. In particular, I have observed:

	B-W(1) S-H(14) B-W(1) THU(2)

What happens:

* row 1 is not already present, dbd_goto_row(1) is executed,
  the row data retrieved, and result->currowidx set to 1
* row 14 is not already present, a goto_row(14) is executed,
  the row data retrieved, and result->currowidx set to 14
* row 1 is already present, result->currowidx set to 1
* row 2 is not already present, dbd_goto_row(2) is NOT executed,
  because of rowidx == currowidx + 1 [2 == 2].
  So the next mysql row fetched is row 15, rather than 2.

result->currowidx is a high-level pointer that always gets updated.
But we do need to track the pointer that the DBD has, as well.

After applying this patch, the seeked results correctly yield:

	B-W(1) S-H(14) B-W(1) BAY(2)
---
 include/dbi/dbi-dev.h | 7 +++++++
 src/dbd_helper.c      | 1 +
 src/dbi_result.c      | 5 ++---
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/dbi/dbi-dev.h b/include/dbi/dbi-dev.h
index 0e21005..175cd7e 100644
--- a/include/dbi/dbi-dev.h
+++ b/include/dbi/dbi-dev.h
@@ -71,7 +71,14 @@ typedef struct dbi_result_s {
 
 	enum { NOTHING_RETURNED, ROWS_RETURNED } result_state;
 	dbi_row_t **rows; /* array of filled rows, elements set to NULL if not fetched yet */
+
+	/* Current row pointer index (1-based) for DBI. */
 	unsigned long long currowidx;
+	/*
+	 * Current row pointer index (1-based) that is synchronized to what
+	 * the current pointer inside the DBD.
+	 */
+	unsigned long long dbd_currowidx;
 } dbi_result_t;
 
 typedef struct _field_binding_s {
diff --git a/src/dbd_helper.c b/src/dbd_helper.c
index 0c03408..d0a9360 100644
--- a/src/dbd_helper.c
+++ b/src/dbd_helper.c
@@ -78,6 +78,7 @@ dbi_result_t *_dbd_result_create(dbi_conn_t *conn, void *handle, unsigned long l
 	result->field_types = NULL;
 	result->field_attribs = NULL;
 	result->result_state = (numrows_matched > 0) ? ROWS_RETURNED : NOTHING_RETURNED;
+	/* result->rows[0] is intentionally unused; rows are in [1..n] */
 	result->rows = calloc(numrows_matched+1, sizeof(dbi_row_t *));
 	result->currowidx = 0;
 
diff --git a/src/dbi_result.c b/src/dbi_result.c
index 751e490..59e64d9 100644
--- a/src/dbi_result.c
+++ b/src/dbi_result.c
@@ -108,7 +108,7 @@ int dbi_result_seek_row(dbi_result Result, unsigned long long rowidx) {
   }
 	
   /* row is one-based for the user, but zero-based to the dbd conn */
-  retval = RESULT->conn->driver->functions->goto_row(RESULT, rowidx-1, RESULT->currowidx-1);
+  retval = RESULT->conn->driver->functions->goto_row(RESULT, rowidx-1, RESULT->dbd_currowidx-1);
   if (retval == -1) {
     _error_handler(RESULT->conn, DBI_ERROR_DBD);
     return 0;
@@ -120,6 +120,7 @@ int dbi_result_seek_row(dbi_result Result, unsigned long long rowidx) {
   }
 
   RESULT->currowidx = rowidx;
+  RESULT->dbd_currowidx = rowidx;
   _activate_bindings(RESULT);
   return retval;
 }
@@ -1789,8 +1790,6 @@ static unsigned int _find_field(dbi_result_t *result, const char *fieldname, dbi
 }
 
 static int _is_row_fetched(dbi_result_t *result, unsigned long long row) {
-  /* Bull patch reported by Tom Lane */
-  /*  if (!result->rows || (row >= result->numrows_matched)) return -1; */
   if (!result->rows || (row > result->numrows_matched)) return -1;
   return !(result->rows[row] == NULL);
 }
-- 
# Created with git-export-patch