|From:||Boszormenyi Zoltan <zb(at)cybertec(dot)at>|
|To:||Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>, Michael Meskes <meskes(at)postgresql(dot)org>|
|Cc:||pgsql-hackers(at)postgresql(dot)org, Hans-Juergen Schoenig <hs(at)cybertec(dot)at>|
|Subject:||Re: Fix auto-prepare #2|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> 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,
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
Cybertec Schönig & Schönig GmbH
|Next Message||Leonardo F||2010-01-19 12:08:55||Re: Review: Patch: Allow substring/replace() to get/set bit values|
|Previous Message||Arnaud Betremieux||2010-01-19 10:54:08||Re: Listen / Notify - what to do when the queue is full|