Re: Review of "SQLDA support for ECPG"

From: Boszormenyi Zoltan <zb(at)cybertec(dot)at>
To: Noah Misch <noah(at)leadboat(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, Michael Meskes <meskes(at)postgresql(dot)org>
Subject: Re: Review of "SQLDA support for ECPG"
Date: 2009-10-05 12:23:57
Message-ID: 4AC9E55D.9050405@cybertec.at
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

thank you very much for the review.

Noah Misch írta:
> I took a look at 2-pg85-sqlda-10-ctxdiff.patch. Starting from CVS HEAD of
> roughly 2009-10-03 05:00 UTC, prerequisite patches 1a-1h applied cleanly.
> 2-pg85-sqlda hit a trivial whitespace reject in ecpg.trailer along with a more
> substantive reject at ecpg.addons:407 (FetchStmtMOVEfetch_args). Fixing it up
> by hand leads to my first question - why did the transition from `opt_ecpg_into'
> to `opt_ecpg_fetch_into' affect FETCH FORWARD and not FETCH BACKWARD?
>

This was a total oversight coming from the previous
dynamic cursorname patch. *That* was buggy as it didn't
have the two de-factorized rules for FETCH BACKWARD.
When I did the fine-grained split-up, I fixed that but I didn't
test my SQLDA patch after the new dynamic cursorname
patches.

I will post a new patch for SQLDA and for all others that need
updating.

> The main test suite acquired no regressions, but I get failures in two tests of
> the ecpg test suite (make -C src/interfaces/ecpg/test check). I attach
> regression.{out,diff} and postmaster.log from the test run. The abort in
> sqlda.pgc looks like the interesting failure, but I exhausted time with which to
> dig into it further. preproc/cursor.pgc creates (and ideally drops) the same
> table `t1' as compat_informix/{sqlda,cursor}.pgc, so a crash in either of the
> others makes it fail too. Could they all use temp tables, use different table
> names, or `DROP TABLE IF EXISTS t1' first?
>
> Do those logs suggest the cause of the sqlda.pgc failure? If not, I will look
> into it further. Otherwise, I'm happy to review a future iteration of the
> patch.
>

No, this is not a real error. I have run into this as well,
which is quickly solved if you execute "make install"
before running "make check" under the ecpg directory.
I guess you already have an installed PG tree at the same
prefix as where you have pointed the new one with the
SQLDA patch applied. Another solution may be to use
a different prefix for the SQLDA source tree.

I start to think that the same remedy would be needed here
as the contrib subdirectory: "make installcheck" is a mental
note the you test the installed libraries, not the freshly compiled
ones under the source directory.

> As a side note, with patch 1* but not patch 2, test_informix entered an infinite
> loop with this error:
> ERROR: syntax error at or near "c" at character 15
> STATEMENT: fetch forward c
>

Do you mean that you applied all the split-up patches posted
for the dynamic cursorname extension? I didn't get this error.
What did you do exactly?

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/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Sabino Mullane 2009-10-05 12:30:57 Re: Rules: A Modest Proposal
Previous Message Simon Riggs 2009-10-05 11:50:55 Re: Hot Standby on git