Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f4xkm-0003tb-IE for pgsql-hackers@arkaria.postgresql.org; Sun, 08 Apr 2018 00:00:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f4xkk-0001b4-CX for pgsql-hackers@arkaria.postgresql.org; Sun, 08 Apr 2018 00:00:14 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f4xkk-0001aJ-5Y for pgsql-hackers@lists.postgresql.org; Sun, 08 Apr 2018 00:00:14 +0000 Received: from tamriel.snowman.net ([96.255.250.162]) by magus.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f4xkY-0003zX-Sh for pgsql-hackers@postgresql.org; Sun, 08 Apr 2018 00:00:12 +0000 Received: by tamriel.snowman.net (Postfix, from userid 1000) id 3F0995F79C; Sat, 7 Apr 2018 20:00:01 -0400 (EDT) Date: Sat, 7 Apr 2018 20:00:01 -0400 From: Stephen Frost To: Fabien COELHO Cc: PostgreSQL Developers Subject: Re: [HACKERS] pgbench - allow to store select results into variables Message-ID: <20180408000001.GD27724@tamriel.snowman.net> References:

<20180407031725.GX27724@tamriel.snowman.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="u/esRqyc5dLeU/8t" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --u/esRqyc5dLeU/8t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Fabien, * Fabien COELHO (coelho@cri.ensmp.fr) wrote: > >> > >>+ > >>+ > >>+ \cset [prefix] or > >>+ \gset [prefix] > >>+ > > > >Seems like this should really be moved down below the section for > >'\set'. >=20 > Dunno. >=20 > I put them there because it is in alphabetical order (for cset at least) = and > because "set" documentation is heavy about expressions (operators, > functions, constants, ...) which have nothing to do with cset/gset, so I > felt that having them clearly separated and in abc order was best. Ah, hmmm, yes, alphabetical order is sensible, certainly. > >>- char *line; /* text of command line */ > >>+ char *line; /* first line for short display */ > >>+ char *lines; /* full multi-line text of command */ > > > >Not really a fan of such closely-named variables... Maybe first_line > >instead? >=20 > Indeed, looks better. Great, thanks. > >>+ case PGRES_TUPLES_OK: > >>+ if (gset[compound] !=3D NULL) > > > >Probably be good to add some comments here, eh: >=20 > >/* > >* The results are intentionally thrown away if we aren't under a gset > >* call. > >*/ >=20 > I added a (shorter) comment. Ok. > >>+ if (ntuples !=3D 1) > >>+ { > >>+ fprintf(stderr, > >>+ "client %d file %d command %d compound %d: " > >>+ "expecting one row, got %d\n", > >>+ st->id, st->use_file, st->command, compound, ntuples); > >>+ st->ecnt++; > >>+ PQclear(res); > >>+ discard_response(st); > >>+ return false; > >>+ } > > > >Might be interesting to support mutli-row (or no row?) returns in the > >future, but not something this patch needs to do, so this looks fine to > >me. >=20 > It could match PL/pgSQL's INTO, that is first row or NULL if none. Yeah, but that's not really something that needs to go into this patch. > >>+ > >>+ /* store result as a string */ > >>+ if (!putVariable(st, "gset", varname, > >>+ PQgetvalue(res, 0, f))) > >>+ { > >>+ /* internal error, should it rather abort? */ > > > >I'm a bit on the fence about if we should abort in this case or not. A > >failure here seems likely to happen consistently (whereas the ntuples > >case might be a fluke of some kind), which tends to make me think we > >should abort, but still thinking about it. >=20 > I'm also still thinking about it:-) For me it is an abort, but there is t= his > whole "return false" internal error management in pgbench the purpose of > which fails me a little bit, so I stick to that anyway. Yeah. > >>+ if (*gset[compound] !=3D '\0') > >>+ free(varname); > > > >A comment here, and above where we're assigning the result of the > >psprintf(), to varname probably wouldn't hurt, explaining that the > >variable is sometimes pointing into the query result structure and > >sometimes not... >=20 > I added two comments to avoid a malloc/free when there are no prefixes. I= STM > that although it might be a border-line over-optimization case, it is a > short one, the free is a few lines away, so it might be ok. Ok, having the comments is definitely good as it was a bit confusing as to what was going on. :) > >>+ /* read and discard the query results */ > > > >That comment doesn't feel quite right now. ;) >=20 > Indeed. Changed with "store or discard". Ok. > >> > >>- /* We don't want to scribble on cmd->argv[0] until done */ > >>- sql =3D pg_strdup(cmd->argv[0]); > >>+ sql =3D pg_strdup(cmd->lines); > > > >The function-header comment for parseQuery() could really stand to be > >improved. >=20 > Indeed. >=20 > >>+ /* merge gset variants into preceeding SQL command */ > >>+ if (pg_strcasecmp(bs_cmd, "gset") =3D=3D 0 || > >>+ pg_strcasecmp(bs_cmd, "cset") =3D=3D 0) > >>+ { > >>+ "\\gset cannot start a script", > >>+ "\\gset must follow a SQL command", > >>+ "\\gset cannot follow one another", > > > >These errors should probably be '\\gset and \\cset' or similar, no? > >Since we fall into this for both.. >=20 > Indeed. >=20 > Attached an improved version, mostly comments and error message fixes. >=20 > I have not changed the 1 row constraint for now. Could be an later > extension. >=20 > If this patch get through, might be handy for simplifying tests in > current pgbench submissions, especially the "error handling" one. Glad to hear that. Unfortunately, I didn't end up having time to wrap this up to a satisfactory level for myself to get it into PG11. I know it's been a long time coming, and thank you for continuing to push on it; I'll try to make sure that I take some time in the first CF for PG12 to wrap this up and get it in. I'm all for these improvements in pgbench in general, it's a really useful tool and it's great to see work going into it. Thanks again! Stephen --u/esRqyc5dLeU/8t Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJayVuAAAoJEO1sijiDR2RVQHcP+wXWjMfNsYfdsUdW3stYs5ma wikOOvpOn5DQoj6A+iOsyu/ifDxhKfUUTwha1LL/ncyh7LMsKaZl8dAohL65j02R FpCTA1BpjFpB7ySrkUXyujRWdbkHlAVPKO9W1+Jtotzx2+ZZPHzvaGOL48OeSkf7 K54megeQ/cF+s+GNmz1VyBScRsICKdj9/dcC6tCYJii5XuRezlt1f1eWlPU0oPm9 z7KVsAscv7z5YdHFaqBJv3PXk8OJB/s725qwWi6awRKhU4XDQ0aLNPi9KAnR2ycy H76PvFz14kElpAdKpI6OHNN9qSy7vdxTv+ZXDZ0LJ7RO7Y481iElfUvINH7SDncN Fc6b5sNs0vK/NXtKrTeSoSJv3L2J8wF8eXypvuXwcitYPDX+Gvn3ef0AvO2h1Pmh Uy7nzTEA2MbhfAb2Mvd9x3lmroK8WdPIEpDK3iKLfVNuew0lZ/P5yO0tbeu3FcvO CiYqlv1b9sa05uPckI8SckpuLf4tPzQ/aNH39XPEewFNx6IMEDAnzL2xXyy47sJX g3R1PTAgMQif1EKjqJXLEHmjh3SizIX4ipraVA/IUYxui0+yHovVR/yyk3Sju0us HZBT8Lv0XZ3xM9/DjPZWMjyBPborXjgfLIKyhqPGEI6oOs9kZTFg9fB2DOlZo4p7 6p/Grq0KzfBoIlrPcQ0k =GtpK -----END PGP SIGNATURE----- --u/esRqyc5dLeU/8t--