Re: Fix auto-prepare #2

Hi,

Takahiro Itagaki írta:
> Hi, I'm reviewing your patch and have a couple of comments.
>

thanks for the review, comments below.

> Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
>
>
>> we have found that auto-prepare causes a problem if the connection
>> is closed and re-opened and the previously prepared query is issued
>> again.
>>
>
> You didn't attach actual test cases for the bug, so I verified it
> by executing the routines twice in ecpg/test/preproc/autoprep.pgc.
> The attached "6-pg85-fix-auto-prepare-multiconn-3-test.patch"
> is an additional regression test for it. Is it enough for your case?
>

Yes, my bad that I didn't attach a test case.
The modification to the autoprep.pgc is enough to trigger it
because the INSERTs are auto-prepared.

>> This fix is that after looking up the query and having it found in the cache
>> we also have to check whether this query was prepared in the current
>> connection. The attached patch implements this fix and solves the problem
>> described above. Also, the missing "return false;" is also added to ECPGdo()
>> in execute.c.
>>
>
> In addition to the two issues, I found memory leaks by careless calls
> of ecpg_strdup() in ecpg_auto_prepare().

Good catch, thanks. I didn't look in ECPGprepare(),
so I just copied the statement and the logic from the ELSE branch.

> Prepared stetements also might
> leak in a error path.

Yes, it is true as well, the statement name ("ecpgN") is not freed
in the error path in ECPGdo().

However, thinking a little more about this code:
con = ecpg_get_connection(connection_name);
prep = ecpg_find_prepared_statement(stmtID, con, NULL);
if (!prep && !ECPGprepare(...))

I only wanted to call ECPGprepare() in case it wasn't already prepared.
ECPGprepare() also checks for the statement being already prepared
with ecpg_find_prepared_statement() but in case it exists it
DEALLOCATEs the statement and PREPAREs again so there's
would be no saving for auto-prepare calling it unconditionally and
we are doing a little extra work by calling ecpg_find_prepared_statement()
twice. We need a common function shared by ECPGprepare() and
ecpg_auto_prepare() to not do extra work in the auto-prepare case.

The attached patch implements this and also your leak fixes
plus includes your change for the autoprep.pgc regression test.

Thanks and best regards,
Zoltán Böszörményi

--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/