Received: from makus.postgresql.org (makus.postgresql.org [98.129.198.125]) by mail.postgresql.org (Postfix) with ESMTP id 76957171224A; Tue, 6 Mar 2012 02:07:58 -0400 (AST) Received: from mail.cybertec.at ([87.118.110.48]) by makus.postgresql.org with esmtp (Exim 4.72) (envelope-from ) id 1S4nYx-0001nW-Qm; Tue, 06 Mar 2012 06:07:58 +0000 Received: from localhost.localdomain (host-87-242-59-39.prtelecom.hu [87.242.59.39]) by mail.cybertec.at (Postfix) with ESMTPSA id 6DC725FC200; Tue, 6 Mar 2012 07:06:55 +0100 (CET) Message-ID: <4F55A9AD.6050600@cybertec.at> Date: Tue, 06 Mar 2012 07:07:41 +0100 From: Boszormenyi Zoltan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.1) Gecko/20120216 Thunderbird/10.0.1 MIME-Version: 1.0 To: Noah Misch CC: Michael Meskes , PG Hackers , Robert Haas , Heikki Linnakangas , Bruce Momjian Subject: Re: ECPG FETCH readahead References: <201006232042.o5NKgb503695@momjian.us> <4C2308FD.1090300@cybertec.at> <4C231F9E.6000102@enterprisedb.com> <20100624121922.GD24137@feivel.credativ.lan> <4CB6D3C8.3020709@cybertec.at> <4EC41434.7010603@cybertec.at> <4EFC36EF.1060308@cybertec.at> <20120302164105.GD23100@tornado.leadboat.com> <4F538B4C.1030605@cybertec.at> <20120305185632.GE13348@tornado.leadboat.com> In-Reply-To: <20120305185632.GE13348@tornado.leadboat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-Pg-Spam-Score: -1.9 (-) X-Archive-Number: 201203/259 X-Sequence-Number: 204141 2012-03-05 19:56 keltez=E9ssel, Noah Misch =EDrta: > > Having pondered the matter further, I now agree with Michael that the f= eature > should stay disabled by default. See my response to him for rationale. > Assuming that conclusion holds, we can recommended a higher value to us= ers who > enable the feature at all. Your former proposal of 256 seems fine. OK. > >> BTW, the default disabled behaviour was to avoid "make check" breakage= , >> see below. >> >>> 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 disa= bles the >>> feature, though one could later re-enable it with ECPGFETCHSZ. >> This means all code previously going through ECPGdo() would go through >> ECPGopen()/ECPGfetch()/ECPGclose(). This is more intrusive and all >> regression tests that were only testing certain features would also >> test the readahead feature, too. > It's a good sort of intrusiveness, reducing the likelihood of introduci= ng bugs > basically unrelated to readahead that happen to afflict only ECPGdo() o= r only > the cursor.c interfaces. Let's indeed not have any preexisting test ca= ses use > readahead per se, but having them use the cursor.c interfaces anyway wi= ll > build confidence in the new code. The churn in expected debug output i= sn't > ideal, but I don't prefer the alternative of segmenting the implementat= ion for > the sake of the test cases. I see. > >> Also, the test for WHERE CURRENT OF at ecpg time would have to be done >> at runtime, possibly making previously working code fail if ECPGFETCHS= Z is enabled. > Good point. > >> How about still allowing "NO READAHEAD" cursors that compile into plai= n ECPGdo()? >> This way, ECPGFETCHSZ don't interfere with WHERE CURRENT OF. But this = would >> mean code changes everywhere where WHERE CURRENT OF is used. > ECPGFETCHSZ should only affect cursors that make no explicit mention of > READAHEAD. I'm not sure whether that should mean actually routing READ= HEAD 1 > cursors through ECPGdo() or simply making sure that cursor.c achieves t= he same > outcome; see later for a possible reason to still do the latter. > >> Or how about a new feature in the backend, so ECPG can do >> UPDATE/DELETE ... WHERE OFFSET N OF cursor >> and the offset of computed from the actual cursor position and the pos= ition known >> by the application? This way an app can do readahead and do work on ro= ws collected >> by the cursor with WHERE CURRENT OF which gets converted to WHERE OFFS= ET OF >> behind the scenes. > That's a neat idea, but I would expect obstacles threatening our abilit= y to > use it automatically for readahead. You would have to make the cursor = a > SCROLL cursor. We'll often pass a negative offset, making the operatio= n fail > if the cursor query used FOR UPDATE. Volatile functions in the query w= ill get > more calls. That's assuming the operation will map internally to somet= hing > like MOVE N; UPDATE ... WHERE CURRENT OF; MOVE -N. You might come up w= ith > innovations to mitigate those obstacles, but those innovations would pr= obably > also apply to MOVE/FETCH. In any event, this would constitute a substa= ntive > patch in its own right. I was thinking along the lines of a Portal keeping the ItemPointerData for each tuple in the last FETCH statement. The WHERE OFFSET N OF cursor would treat the offset value relative to the tuple order returned by FETC= H. So, OFFSET 0 OF =3D=3D CURRENT OF and other values of N are negative. This way, it doesn't matter if the cursor is SCROLL, NO SCROLL or have the default behaviour with "SCROLL in some cases". Then ECPGopen() doesn't have to play games with the DECLARE statement. Only ECPGfetch() needs to play with MOVE statements, passing different offsets to the back= end, not what the application passed. > One way out of trouble here is to make WHERE CURRENT OF imply READHEAD > 1/READHEAD 0 (incidentally, perhaps those two should be synonyms) on th= e > affected cursor. If the cursor has some other readahead quantity decla= red > explicitly, throw an error during preprocessing. I played with this idea a while ago, from a different point of view. If the ECPG code had the DECLARE mycur, DML ... WHERE CURRENT OF mycur and OPEN mycur in exactly this order, i.e. WHERE CURRENT OF appears in a standalone function between DECLARE and the first OPEN for the cursor, then ECPG disabled readahead automatically for that cursor and for that cursor only. But this requires effort on the user of ECPG and can be very fragile. Code cleanup with reordering functions can break previously working code. > Failing a reasonable resolution, I'm prepared to withdraw my suggestion= of > making ECPGFETCHSZ always-usable. It's nice to have, not critical. > >>>> +bool >>>> +ECPGopen(const int lineno, const int compat, const int force_indica= tor, >>>> + 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 =3D ECPGget_sqlca(); >>>> + >>>> + if (!query) >>>> + { >>>> + ecpg_raise(lineno, ECPG_EMPTY, ECPG_SQLSTATE_ECPG_INTERNAL_ERROR,= NULL); >>>> + return false; >>>> + } >>>> + ptr =3D strstr(query, "for "); >>>> + if (!ptr) >>>> + { >>>> + ecpg_raise(lineno, ECPG_INVALID_STMT, ECPG_SQLSTATE_ECPG_INTERNAL= _ERROR, NULL); >>>> + return false; >>>> + } >>>> + whold =3D strstr(query, "with hold "); >>>> + dollar0 =3D strstr(query, "$0"); >>>> + >>>> + noscroll =3D strstr(query, "no scroll "); >>>> + scroll =3D strstr(query, "scroll "); >>> A query like 'SELECT 1 AS "with hold "' fools these lexical tests. >> But SELECT 1 AS "with hold" doesn't go through ECPGopen(), it's run by= ECPGdo() >> so no breakage there. ecpglib functions are not intended to be called = from manually >> constructed C code. > I tried something like > EXEC SQL DECLARE cur CURSOR FOR SELECT * FROM generate_series(1 , $1) = AS t("with hold "); > It wrongly generated this backend command: > declare cur no scroll cursor with hold for select * from generate_seri= es ( 1 , $1 ) as t ( "with hold " ) Ah, ok. The grammar test in ecpg is better. >>> An unusable ECPGFETCHSZ should procedure an error, not >>> silently give no effect. >> Point taken. Which error handling do imagine? abort() or simply return= ing false >> and raise and error in SQLCA? > The latter. > >>>> + /* >>>> + * 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] =3D=3D 0 */) >>> Why is the commented code there? >> Some leftover from testing, it shouldn't be there. Actually, now I remember. It was a wishful thinking on my part that whenever PG supports returning a number of rows at opening the cursor, correct or estimated, this code shouldn't be executed, just accept what the backend has given us. >> >>>> + { >>>> + struct connection *con =3D ecpg_get_connection(connection_name); >>>> + struct cursor_descriptor *cur; >>>> + bool existing; >>>> + int64 n_tuples; >>>> + >>>> + if (scrollable) >>>> + { >>>> + PGresult *res; >>>> + char *query; >>>> + char *endptr =3D NULL; >>>> + >>>> + query =3D ecpg_alloc(strlen(curname) + strlen("move all in ") + = 2, lineno); >>>> + sprintf(query, "move all in %s", curname); >>>> + res =3D PQexec(con->connection, query); >>>> + n_tuples =3D strtoull(PQcmdTuples(res), &endptr, 10); >>>> + PQclear(res); >>>> + ecpg_free(query); >>>> + >>>> + /* Go back to the beginning of the resultset. */ >>>> + query =3D ecpg_alloc(strlen(curname) + strlen("move absolute 0 i= n ") + 2, lineno); >>>> + sprintf(query, "move absolute 0 in %s", curname); >>>> + res =3D PQexec(con->connection, query); >>>> + PQclear(res); >>>> + ecpg_free(query); >>>> + } >>>> + else >>>> + { >>>> + n_tuples =3D 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 records= et, >>>> so the previous ECPG "deficiency" (backend limitation) that sqlca.sq= lerrd[2] >>>> didn't report the (possibly estimated) number of rows in the results= et >>>> 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 no= t >>> populate sqlerrd[2] this way: >>> http://publib.boulder.ibm.com/infocenter/idshelp/v10/index.jsp?topic=3D= /com.ibm.sqlt.doc/sqltmst189.htm >>> http://docs.oracle.com/cd/A57673_01/DOC/api/doc/PC_22/ch10.htm#toc139 >> The problem here is that Informix in the field in fact returns the num= ber of rows >> in the cursor and the customer we developed this readahead code for re= lied on this. >> Maybe this was eliminated in newer versions of Informix to make it fas= ter. >> >>> The performance impact will vary widely depending on the query cost p= er row >>> and the fraction of rows the application will actually retrieve. Con= sider a >>> complex aggregate returning only a handful of rows. >> Indeed. >> >>> Consider SELECT * on a >>> 1B-row table with the application ceasing reads after 1000 rows. Per= formance >>> aside, this will yield double execution of any volatile functions inv= olved. >>> So, I think we ought to diligently avoid this step. (Failing that, t= he >>> documentation must warn about the extra full cursor scan and this fea= ture must >>> stay disabled by default.) >> OK, how about enabling it for Informix-compat mode only, or only via a= n >> environment variable? I agree it should be documented. > For a query where backend execution cost dominates the cost of transfer= ring > rows to the client, does Informix take roughly twice the normal time to > execute the query via an ESQL/C cursor? Is that acceptable overhead fo= r every > "ecpg -C" user? (FWIW, I've never used Informix-compat mode.) If not,= the > feature deserves its own option. > > Whatever the trigger condition, shouldn't this apply independent of whe= ther > readahead is in use for a given cursor? I guess so. > (This could constitute a reason to > use the cursor.c interfaces for every cursor.) Indeed. > Does some vendor-neutral standard define semantics for sqlerrd, or has = it > propagated by imitation? No idea. > This reminds me to mention: your documentation should note that the use= of > readahead or the option that enables sqlerrd[2] calculation may change = the > outcome of queries calling volatile functions. See how the DECLARE > documentation page discusses this hazard for SCROLL/WITH HOLD cursors. OK. > >>>> --- 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/i= nclude. >> OK. >> >>> dollarzero has something to do with dynamic cursor names, right? Doe= s it have >>> other roles? >> Yes, it had other roles. ECPG supports user variables in cases where t= he >> PostgreSQL grammar doesn't. There's this rule: >> >> ECPG: var_valueNumericOnly addon >> if ($1[0] =3D=3D '$') >> { >> free($1); >> $1 =3D mm_strdup("$0"); >> } >> >> The "var_value: NumericOnly" case in gram.y can show up in a lot of ca= ses. >> This feature was there before the dynamic cursor. You can even use the= m together >> which means more than one $0 placeholders in the statement. E.g.: >> FETCH :amount FROM :curname; >> gets translated to >> FETCH $0 FROM $0; >> by ecpg, and both the amount and the cursor name is passed in in user = variables. >> The value is needed by cursor.c, this is why this "dollarzero" pointer= is needed. > Thanks for that explanation; the situation is clearer to me now. > > nm > --=20 ---------------------------------- Zolt=E1n B=F6sz=F6rm=E9nyi Cybertec Sch=F6nig & Sch=F6nig GmbH Gr=F6hrm=FChlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/