Message-ID: <4F81BE55.1010904@cybertec.at>
Date: Sun, 08 Apr 2012 18:35:33 +0200
From: Boszormenyi Zoltan <zb@cybertec.at>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
	rv:11.0) Gecko/20120329 Thunderbird/11.0.1
MIME-Version: 1.0
To: Noah Misch <noah@leadboat.com>, Michael Meskes <meskes@postgresql.org>,
	Robert Haas <robertmhaas@gmail.com>,
	PG Hackers <pgsql-hackers@postgresql.org>,
	Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>,
	Bruce Momjian <bruce@momjian.us>
Subject: Re: ECPG FETCH readahead
References: <4F55A9AD.6050600@cybertec.at>
	<20120306110658.GC15988@tornado.leadboat.com>
	<CA+TgmoaBGLpW5V3nFES-McLbZyY_9nUvpnFLjT7zAYuVPu809A@mail.gmail.com>
	<4F6D9893.3030300@cybertec.at>
	<20120329004323.GA17329@tornado.leadboat.com>
	<4F74409C.3050904@cybertec.at>
	<20120329170341.GA4142@tornado.leadboat.com>
	<4F74E6A7.8040204@cybertec.at>
	<20120407112008.GA2286@feivel.credativ.lan>
	<20120407155042.GB11987@tornado.leadboat.com>
	<20120408142501.GA21513@feivel.credativ.lan>
In-Reply-To: <20120408142501.GA21513@feivel.credativ.lan>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable

2012-04-08 16:25 keltez=C3=A9ssel, Michael Meskes =C3=ADrta:
> On Sat, Apr 07, 2012 at 11:50:42AM -0400, Noah Misch wrote:
>> Both.  The second patch appeared after my first review, based on a com=
ment in
>> that review.  I looked at it during my re-review before marking the ov=
erall
>> project Ready for Committer.
> Thanks.
>
>> I do call your attention to a question I raised in my second review: i=
f a
>> program contains "DECLARE foo READAHEAD 5 CURSOR FOR ..." and the user=
 runs
>> the program with ECPGFETCHSZ=3D10 in the environment, should that curs=
or use a
>> readahead window of 5 or of 10?  Original commentary:
>> http://archives.postgresql.org/message-id/20120329004323.GA17329@torna=
do.leadboat.com
> I'd say it should be 5. I don't like an environment variable overwritin=
g a
> hard-coded setting. I think this is what you, Noah, thought, too, right=
? I'd
> say let's change this.

Do you want me to change this or will you do it? I am on holiday
and will be back to work on wednesday.

The possibility to test different readahead window sizes
without modifying the source and recompiling was useful.

>   Is it possible to allow just READAHEAD without a number?
> In that case I would accept the environment variable.

After the 2nd patch is applied, this is exactly the case.
The cursors are driven and accounted using the new functions
but with the readahead window being a single row as the default
value for fetch_readahead is 1.


>
> And some comments mostly directed at Zoltan:
>
> ecpg --help says ...default 0 (disabled)..., but options less than 1 ar=
e not
> accepted and the default setting of 1 has a comment "Disabled by defaul=
t". I
> guess this needs to be adjusted.

Yes, the help text was not changed in the 2nd patch, I missed that.

>
> Is there a reason why two new options for ecpg were invented? Normally =
ecpg
> options define how the preprocessor works but not the resulting binary.

The -R option simply provides a default without ornamenting
the DECLARE statement.

>   Well,
> different preprocessor behaviour might result in different binary behav=
iour of
> course. The only option that only effects the resulting binary is "-r" =
for
> "runtime". Again, this is not completely true as the option has to make=
 its way
> into the binary, but that's it. Now I wonder whether it would make more=
 sense
> to add the two options as runtime options instead. The
> --detect-cursor-resultset-size option should work there without a probl=
em.

You are right. This can be a suboption to "-r".

>   I
> haven't delved into the source code enough to find out if -R changes so=
mething
> in the compiler stage.

"-R" works just like "-r" in the sense that a value gets passed
to the runtime. "-R" simply changes the default value that gets
passed if no READAHEAD N clause is specified for a cursor.
This is true only if you intend to apply both paches.

Without the 2nd patch and fetch_readahead=3D0 (no -R option given)
or NO READAHEAD is specified for a cursor, the compiler makes
a distinction between uncachable cursors driven by ECPGdo() and
cachable cursors driven by the new runtime functions.

With the 2nd patch applied, this distinction is no more.

>
> The test case cursor-readahead.pgc has a comment saying "test automatic=
 prepare
> for all statements". Copy/Paste error?

It must be, yes.

> I cannot find a test that tests the environment variable giving the fet=
ch size.
> Could you please point me to that?

I didn't write such a test. The reason is that while variables are
exported by make from the Makefile to the binaries run by make
e.g.  CFLAGS et.al. for $(CC), "make check" simply runs pg_regress
once which uses its own configuration file that doesn't have a
way to set or unset an environment variable. This could be a useful
extension to pg_regress though.

Best regards,
Zolt=C3=A1n B=C3=B6sz=C3=B6rm=C3=A9nyi

>
> Michael


--=20
----------------------------------
Zolt=C3=A1n B=C3=B6sz=C3=B6rm=C3=A9nyi
Cybertec Sch=C3=B6nig&  Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/