Re: ECPG FETCH readahead

On Thu, Dec 29, 2011 at 10:46:23AM +0100, Boszormenyi Zoltan wrote:
> 2011-11-16 20:51 keltez?ssel, Boszormenyi Zoltan ?rta:
> > 2010-10-14 11:56 keltez?ssel, Boszormenyi Zoltan ?rta:
> >>> On Thu, Jun 24, 2010 at 8:19 AM, Michael Meskes <meskes(at)postgresql(dot)org> wrote:
> >>>> On Thu, Jun 24, 2010 at 12:04:30PM +0300, Heikki Linnakangas wrote:
> >>>>> Is there a reason not to enable it by default? I'm a bit worried
> >>>>> that it will receive no testing if it's not always on.
> >>>>>
> >>>> Yes, there is a reason, namely that you don't need it in native mode at all.
> >>>> ECPG can read as many records as you want in one go, something ESQL/C
> >>>> apparently cannot do. This patch is a workaround for that restriction. I still
> >>>> do not really see that this feature gives us an advantage in native mode
> >>>> though.

We yet lack a consensus on whether native ECPG apps should have access to the
feature. My 2c is to make it available, because it's useful syntactic sugar.
If your program independently processes each row of an arbitrary-length result
set, current facilities force you to add an extra outer loop to batch the
FETCHes for every such code site. Applications could define macros to
abstract that pattern, but this seems common-enough to justify bespoke
handling. Besides, minimalists already use libpq directly.

I suggest enabling the feature by default but drastically reducing the default
readahead chunk size from 256 to, say, 5. That still reduces the FETCH round
trip overhead by 80%, but it's small enough not to attract pathological
behavior on a workload where each row is a 10 MiB document. I would not offer
an ecpg-time option to disable the feature per se. Instead, let the user set
the default chunk size at ecpg time. A setting of 1 effectively disables the
feature, though one could later re-enable it with ECPGFETCHSZ.

> > The ASAP took a little long. The attached patch is in git diff format,
> > because (1) the regression test intentionally doesn't do ECPGdebug()
> > so the patch isn't dominated by a 2MB stderr file, so this file is empty
> > and (2) regular diff cannot cope with empty new files.

Avoid the empty file with fprintf(STDERR, "Dummy non-empty error output\n");

> > - *NEW FEATURE* Readahead can be individually enabled or disabled
> > by ECPG-side grammar:
> > DECLARE curname [ [ NO ] READAHEAD ] CURSOR FOR ...
> > Without [ NO ] READAHEAD, the default behaviour is used for cursors.

Likewise, this may as well take a chunk size rather than a yes/no.

The patch adds warnings:
error.c: In function `ecpg_raise':
error.c:281: warning: format not a string literal and no format arguments
error.c:281: warning: format not a string literal and no format arguments

The patch adds few comments and no larger comments explaining its higher-level
ideas. That makes it much harder to review. In this regard it follows the
tradition of the ECPG code, but let's depart from that tradition for new work.
I mention a few cases below where the need for commentary is acute.

I tested full reads of various "SELECT * FROM generate_series(1, $1)" commands
over a 50ms link, and the patch gives a sound performance improvement. With
no readahead, a mere N=100 takes 5s to drain. With readahead enabled, N=10000
takes only 3s. Performance was quite similar to that of implementing my own
batching with "FETCH 256 FROM cur". When I kicked up ECPGFETCHSZ (after
fixing its implementation -- see below) past the result set size, performance
was comparable to that of simply passing the query through psql.

> --- a/doc/src/sgml/ecpg.sgml
> +++ b/doc/src/sgml/ecpg.sgml

> @@ -5289,6 +5315,17 @@ while (1)
> </varlistentry>
>
> <varlistentry>
> + <term>-231 (<symbol>ECPG_INVALID_CURSOR</symbol>)</term>
> + <listitem>
> + <para>
> + The cursor you are trying to use with readahead has not been opened yet (SQLSTATE 34000),
> + invalid values were passed to libecpg (SQLSTATE 42P11) hor not in prerequisite state, i.e.

Typo.

> --- /dev/null
> +++ b/src/interfaces/ecpg/ecpglib/cursor.c
> @@ -0,0 +1,730 @@

cursor.c contains various >78-col lines. pgindent has limited ability to
improve those, so please split them.

> +static struct cursor_descriptor *
> +add_cursor(int lineno, struct connection *con, const char *name, bool scrollable, int64 n_tuples, bool *existing)
> +{
> + struct cursor_descriptor *desc,
> + *ptr, *prev = NULL;
> + bool found = false;
> +
> + if (!name || name[0] == '\0')
> + {
> + if (existing)
> + *existing = false;
> + return NULL;
> + }
> +
> + ptr = con->cursor_desc;
> + while (ptr)
> + {
> + int ret = strcasecmp(ptr->name, name);
> +
> + if (ret == 0)
> + {
> + found = true;
> + break;
> + }
> + if (ret > 0)
> + break;
> +
> + prev = ptr;
> + ptr = ptr->next;
> + }

Any reason not to use find_cursor() here?

> +static void
> +del_cursor(struct connection *con, const char *name)
> +{
> + struct cursor_descriptor *ptr, *prev = NULL;
> + bool found = false;
> +
> + ptr = con->cursor_desc;
> + while (ptr)
> + {
> + int ret = strcasecmp(ptr->name, name);
> +
> + if (ret == 0)
> + {
> + found = true;
> + break;
> + }
> + if (ret > 0)
> + break;
> +
> + prev = ptr;
> + ptr = ptr->next;
> + }

Any reason not to use find_cursor() here?

> +bool
> +ECPGopen(const int lineno, const int compat, const int force_indicator,
> + const char *connection_name, const bool questionmarks,
> + const char *curname, const int st, const char *query, ...)
> +{
> + va_list args;
> + bool ret, scrollable;
> + char *new_query, *ptr, *whold, *noscroll, *scroll, *dollar0;
> + struct sqlca_t *sqlca = ECPGget_sqlca();
> +
> + if (!query)
> + {
> + ecpg_raise(lineno, ECPG_EMPTY, ECPG_SQLSTATE_ECPG_INTERNAL_ERROR, NULL);
> + return false;
> + }
> + ptr = strstr(query, "for ");
> + if (!ptr)
> + {
> + ecpg_raise(lineno, ECPG_INVALID_STMT, ECPG_SQLSTATE_ECPG_INTERNAL_ERROR, NULL);
> + return false;
> + }
> + whold = strstr(query, "with hold ");
> + dollar0 = strstr(query, "$0");
> +
> + noscroll = strstr(query, "no scroll ");
> + scroll = strstr(query, "scroll ");

A query like 'SELECT 1 AS "with hold "' fools these lexical tests. Capture
that information in the parser rather than attempting to reconstruct it here.

> + scrollable = (noscroll == NULL) && (scroll != NULL) && (scroll < ptr);
> +
> + new_query = ecpg_alloc(strlen(curname) + strlen(ptr) + (whold ? 10 : 0) + 32, lineno);
> + if (!new_query)
> + return false;
> + sprintf(new_query, "declare %s %s cursor %s%s",
> + (dollar0 && (dollar0 < ptr) ? "$0" : curname),
> + (scrollable ? "scroll" : "no scroll"),
> + (whold ? "with hold " : ""),
> + ptr);
> +
> + /* Set the fetch size the first time we are called. */
> + if (fetch_size == 0)
> + {
> + char *fsize_str = getenv("ECPGFETCHSZ");
> + char *endptr = NULL;
> + int fsize;
> +
> + if (fsize_str)
> + {
> + fsize = strtoul(fsize_str, &endptr, 10);
> + if (endptr || (fsize < 4))
> + fetch_size = DEFAULTFETCHSIZE;

"endptr" will never be NULL; use "*endptr". As it stands, the code always
ignores ECPGFETCHSZ. An unusable ECPGFETCHSZ should procedure an error, not
silently give no effect. Why a minimum of 4?

> + else
> + fetch_size = fsize;
> + }
> + else
> + fetch_size = DEFAULTFETCHSIZE;
> + }
> +
> + va_start(args, query);
> + ret = ecpg_do(lineno, compat, force_indicator, connection_name, questionmarks, st, new_query, args);
> + va_end(args);
> +
> + ecpg_free(new_query);
> +
> + /*
> + * If statement went OK, add the cursor and discover the
> + * number of rows in the recordset. This will slow down OPEN
> + * but we gain a lot with caching.
> + */
> + if (ret /* && sqlca->sqlerrd[2] == 0 */)

Why is the commented code there?

> + {
> + struct connection *con = ecpg_get_connection(connection_name);
> + struct cursor_descriptor *cur;
> + bool existing;
> + int64 n_tuples;
> +
> + if (scrollable)
> + {
> + PGresult *res;
> + char *query;
> + char *endptr = NULL;
> +
> + query = ecpg_alloc(strlen(curname) + strlen("move all in ") + 2, lineno);
> + sprintf(query, "move all in %s", curname);
> + res = PQexec(con->connection, query);
> + n_tuples = strtoull(PQcmdTuples(res), &endptr, 10);
> + PQclear(res);
> + ecpg_free(query);
> +
> + /* Go back to the beginning of the resultset. */
> + query = ecpg_alloc(strlen(curname) + strlen("move absolute 0 in ") + 2, lineno);
> + sprintf(query, "move absolute 0 in %s", curname);
> + res = PQexec(con->connection, query);
> + PQclear(res);
> + ecpg_free(query);
> + }
> + else
> + {
> + n_tuples = 0;
> + }

You give this rationale for the above code:

On Thu, Jun 17, 2010 at 02:09:47PM +0200, Boszormenyi Zoltan wrote:
> ECPGopen() also discovers the total number of records in the recordset,
> so the previous ECPG "deficiency" (backend limitation) that sqlca.sqlerrd[2]
> didn't report the (possibly estimated) number of rows in the resultset
> is now
> overcome. This slows down OPEN for cursors serving larger datasets
> but it makes possible to position the readahead window using MOVE
> ABSOLUTE no matter what FORWARD/BACKWARD/ABSOLUTE/RELATIVE
> variants are used by the application. And the caching is more than
> overweighs
> the slowdown in OPEN it seems.

From the documentation for Informix and Oracle, those databases do not
populate sqlerrd[2] this way:
http://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=/com.ibm.sqlt.doc/sqltmst189.htm
http://docs.oracle.com/cd/A57673_01/DOC/api/doc/PC_22/ch10.htm#toc139

The performance impact will vary widely depending on the query cost per row
and the fraction of rows the application will actually retrieve. Consider a
complex aggregate returning only a handful of rows. Consider SELECT * on a
1B-row table with the application ceasing reads after 1000 rows. Performance
aside, this will yield double execution of any volatile functions involved.
So, I think we ought to diligently avoid this step. (Failing that, the
documentation must warn about the extra full cursor scan and this feature must
stay disabled by default.)

> +
> + /* Add the cursor */
> + cur = add_cursor(lineno, con, curname, scrollable, n_tuples, &existing);
> +
> + /*
> + * Report the number of tuples for the [scrollable] cursor.
> + * The server didn't do it for us.
> + */
> + sqlca->sqlerrd[2] = (cur->n_tuples < LONG_MAX ? cur->n_tuples : LONG_MAX);
> + }
> +
> + return ret;
> +}
> +
> +static bool
> +ecpg_cursor_execute(struct statement * stmt, struct cursor_descriptor *cur)
> +{
> + char tmp[64];
> + char *query;
> + int64 start_pos;
> +
> + if ((cur->cache_pos >= cur->start_pos) && cur->res && (cur->cache_pos < cur->start_pos + PQntuples(cur->res)))
> + {
> + stmt->results = cur->res;
> + ecpg_free_params(stmt, true, stmt->lineno);
> + return true;
> + }

Why does ecpg_cursor_execute() also call ecpg_free_params()? Offhand, it
seems that ECPGfetch() always takes care of that and is the more appropriate
place, seeing it's the one calling ecpg_build_params().

> --- a/src/interfaces/ecpg/ecpglib/data.c
> +++ b/src/interfaces/ecpg/ecpglib/data.c
> @@ -120,7 +120,7 @@ check_special_value(char *ptr, double *retval, char **endptr)
> }
>
> bool
> -ecpg_get_data(const PGresult *results, int act_tuple, int act_field, int lineno,
> +ecpg_get_data(const PGresult *results, int var_index, int act_tuple, int act_field, int lineno,
> enum ECPGttype type, enum ECPGttype ind_type,
> char *var, char *ind, long varcharsize, long offset,
> long ind_offset, enum ARRAY_TYPE isarray, enum COMPAT_MODE compat, bool force_indicator)

This function could sure use a block comment such as would be customary in
src/backend. Compare the one at heap_update(), for example.

> --- a/src/interfaces/ecpg/ecpglib/error.c
> +++ b/src/interfaces/ecpg/ecpglib/error.c
> @@ -268,6 +268,20 @@ ecpg_raise(int line, int code, const char *sqlstate, const char *str)
> ecpg_gettext("could not connect to database \"%s\" on line %d"), str, line);
> break;
>
> + case ECPG_INVALID_CURSOR:
> + if (strcmp(sqlstate, ECPG_SQLSTATE_OBJECT_NOT_IN_PREREQUISITE_STATE) == 0)
> + snprintf(sqlca->sqlerrm.sqlerrmc, sizeof(sqlca->sqlerrm.sqlerrmc),
> +
> + /*
> + * translator: this string will be truncated at 149 characters
> + * expanded.
> + */
> + ecpg_gettext("cursor can only scan forward"));

Every other message works in the line number somehow; this should do the same.

> --- a/src/interfaces/ecpg/ecpglib/execute.c
> +++ b/src/interfaces/ecpg/ecpglib/execute.c

> @@ -1707,46 +1809,20 @@ ecpg_execute(struct statement * stmt)
> }
>
> bool
> -ECPGdo(const int lineno, const int compat, const int force_indicator, const char *connection_name, const bool questionmarks, const int st, const char *query,...)
> +ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
> + const char *connection_name, const bool questionmarks,
> + enum ECPG_statement_type statement_type, const char *query,
> + va_list args, struct statement **stmt_out)

A block comment would especially help this function, considering the name
tells one so little.

> --- a/src/interfaces/ecpg/ecpglib/extern.h
> +++ b/src/interfaces/ecpg/ecpglib/extern.h
> @@ -60,6 +60,12 @@ struct statement
> bool questionmarks;
> struct variable *inlist;
> struct variable *outlist;
> + char *oldlocale;
> + const char **dollarzero;
> + int ndollarzero;
> + const char **param_values;
> + int nparams;
> + PGresult *results;
> };

Please comment the members of this struct like we do in most of src/include.
dollarzero has something to do with dynamic cursor names, right? Does it have
other roles?

> --- a/src/interfaces/ecpg/preproc/ecpg.header
> +++ b/src/interfaces/ecpg/preproc/ecpg.header

> @@ -111,6 +115,26 @@ mmerror(int error_code, enum errortype type, const char *error, ...)
> }
>
> /*
> + * set use_fetch_readahead based on the current cursor
> + * doesn't return if the cursor is not declared
> + */
> +static void
> +set_cursor_readahead(const char *curname)
> +{
> + struct cursor *ptr;
> +
> + for (ptr = cur; ptr != NULL; ptr = ptr->next)
> + {
> + if (strcmp(ptr->name, curname) == 0)
> + break;
> + }
> + if (!ptr)
> + mmerror(PARSE_ERROR, ET_FATAL, "cursor \"%s\" does not exist", curname);
> +
> + use_fetch_readahead = ptr->fetch_readahead;
> +}

Following add_additional_variables(), use strcasecmp() for literal cursor
names and strcmp() for cursor name host variables.

> --- a/src/interfaces/ecpg/preproc/extern.h
> +++ b/src/interfaces/ecpg/preproc/extern.h
> @@ -24,12 +24,17 @@ extern bool autocommit,
> force_indicator,
> questionmarks,
> regression_mode,
> - auto_prepare;
> + auto_prepare,
> + fetch_readahead;
> +extern bool use_fetch_readahead;

The names of the last two variables don't make clear the difference between
them. I suggest default_fetch_readahead and current_fetch_readahead.

> --- /dev/null
> +++ b/src/interfaces/ecpg/test/preproc/cursor-readahead.pgc
> @@ -0,0 +1,244 @@
> +#include <stdio.h>
> +#include <malloc.h>

Why <malloc.h>? I only see this calling free(); use <stdlib.h> instead.

Thanks,
nm