Re: ECPG FETCH readahead - Mailing list pgsql-hackers

2012-04-08 16:25 keltezéssel, Michael Meskes írta:
> 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 comment in
>> that review.  I looked at it during my re-review before marking the overall
>> project Ready for Committer.
> Thanks.
>
>> I do call your attention to a question I raised in my second review: if a
>> program contains "DECLARE foo READAHEAD 5 CURSOR FOR ..." and the user runs
>> the program with ECPGFETCHSZ=10 in the environment, should that cursor use a
>> readahead window of 5 or of 10?  Original commentary:
>> http://archives.postgresql.org/message-id/20120329004323.GA17329@tornado.leadboat.com
> I'd say it should be 5. I don't like an environment variable overwriting 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 are not
> accepted and the default setting of 1 has a comment "Disabled by default". 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 behaviour 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 problem.

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 something
> 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=0 (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 fetch 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án Böszörményi

>
> Michael


--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig&  Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de     http://www.postgresql.at/