Review of "SQLDA support for ECPG"

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?

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.

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

Thank you,
nm

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/

Hi,

Boszormenyi Zoltan írta:
> I will post a new patch for SQLDA and for all others that need
> updating.
>

Here it is. Added the changes for the FETCH BACKWARD
rules and fixed the other reject as well.

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/

On Mon, Oct 05, 2009 at 02:23:57PM +0200, Boszormenyi Zoltan wrote:
> Noah Misch ?rta:

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

Thanks; that one does apply cleanly.

> > 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).

> 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.

This was exactly the problem; with an unoccupied prefix, all the tests do pass.

> > 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?

Having started over from a clean base tree, I can no longer reproduce this.
Another operator error, no doubt.

All tests now pass here with 1a-1h of "Dynamic cursor support for ECPG" alone,
with "SQLDA support for ECPG" also applied, and with "DESCRIBE [OUTPUT] support
for ECPG" additionally. I will report on the sqlda patch in more detail on
2009-10-10. The one concern that's clear now is a lack of documentation update.

Thank you,
nm

Noah Misch írta:
> I will report on the sqlda patch in more detail on
> 2009-10-10.

I am attaching a new one, please review this. Changes:
- set sqllen, sqlind, sqlilen and sqlitype per-field properties
- SQLINT8 and SQLSERIAL8 are ECPGt_long on 64-bit, not ECPGt_long_long

> The one concern that's clear now is a lack of documentation update.
>

That will come in a separate patch.

Thanks,
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/

On Thu, Oct 8, 2009 at 6:22 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> Noah Misch írta:
>> I will report on the sqlda patch in more detail on
>> 2009-10-10.
>
> I am attaching a new one, please review this. Changes:
> - set sqllen, sqlind, sqlilen and sqlitype per-field properties
> - SQLINT8 and SQLSERIAL8 are ECPGt_long on 64-bit, not ECPGt_long_long
>
>>   The one concern that's clear now is a lack of documentation update.
>>
>
> That will come in a separate patch.

What's the point of that? It can't be applied without documentation,
and it just makes life more complicated to have two separate patch
files floating around.

...Robert

Robert Haas írta:
> On Thu, Oct 8, 2009 at 6:22 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
>
>> Noah Misch írta:
>>
>>> I will report on the sqlda patch in more detail on
>>> 2009-10-10.
>>>
>> I am attaching a new one, please review this. Changes:
>> - set sqllen, sqlind, sqlilen and sqlitype per-field properties
>> - SQLINT8 and SQLSERIAL8 are ECPGt_long on 64-bit, not ECPGt_long_long
>>
>>
>>> The one concern that's clear now is a lack of documentation update.
>>>
>>>
>> That will come in a separate patch.
>>
>
> What's the point of that? It can't be applied without documentation,
> and it just makes life more complicated to have two separate patch
> files floating around.
>
> ...Robert
>

It's easier to write the documentation for all changes at once.
I would have the same situation that happened with the code,
the patches with the documentation added would strictly depend
on each other again. Also, Michael Meskes applied the "string"
pseudo-type patch without the documentation, despite the patch
had it, maybe at an improper place. With a tongue-in-cheek
"no comment" ;-) I point to this paragraph in the ECPG part of
the documentation:

"This documentation is quite incomplete. But since this interface is
standardized,
additional information can be found in many resources about SQL."

The doc patch is attached. Details:
- documented "string" pseudo-type
- documented two more statements under section
"Additional embedded SQL statements" which is now called
"Additional/missing embedded SQL statements"
This is about the current state of ECPG, no code changed.
- document that INTO and USING are interchangeable if
the target is a Descriptor Areas
- document that the SQL keyword is not optional for DAs
- document SQLDA
- document DESCRIBE in section "Using SQL Descriptor Areas"

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/

Boszormenyi Zoltan írta:
> Noah Misch írta:
>
>> I will report on the sqlda patch in more detail on
>> 2009-10-10.
>>
>
> I am attaching a new one, please review this. Changes:
> - set sqllen, sqlind, sqlilen and sqlitype per-field properties
> - SQLINT8 and SQLSERIAL8 are ECPGt_long on 64-bit, not ECPGt_long_long
>

New one attached, the second one is now really fixed, sorry
for the previous one.

--
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/

On Thu, Oct 8, 2009 at 7:15 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> It's easier to write the documentation for all changes at once.
> I would have the same situation that happened with the code,
> the patches with the documentation added would strictly depend
> on each other again. Also, Michael Meskes applied the "string"
> pseudo-type patch without the documentation, despite the patch
> had it, maybe at an improper place. With a tongue-in-cheek
> "no comment" ;-) I point to this paragraph in the ECPG part of
> the documentation:
>
> "This documentation is quite incomplete. But since this interface is
> standardized,
> additional information can be found in many resources about SQL."

OK, maybe I was overly optimistic. :-(

At least for parts of PostgreSQL other than ECPG, it is our usual
practice to require documentation to be submitted with the patch.

I have not looked at your patches and am not familiar with ECPG, but I
wonder if part of the issue here is that there are too many
interrelated changes. Maybe you'd be better off submitting some
smaller changes, wait to see how they get committed, and then move on
to the next thing. On the other hand, given that Michael seems to
have no time to review ECPG patches or provide feedback, and given
that none of the other committers seem to want to touch this with a
ten-foot-pole, maybe that would make this take forever.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I have not looked at your patches and am not familiar with ECPG, but I
> wonder if part of the issue here is that there are too many
> interrelated changes. Maybe you'd be better off submitting some
> smaller changes, wait to see how they get committed, and then move on
> to the next thing. On the other hand, given that Michael seems to
> have no time to review ECPG patches or provide feedback, and given
> that none of the other committers seem to want to touch this with a
> ten-foot-pole, maybe that would make this take forever.

I presume that at some point Michael will have more free time than
he does now; we cannot expect him to do the work exactly in sync with
an arbitrarily-scheduled commit fest. Since ECPG stuff isn't really
blocking anything else, I have no problem with waiting till he does
have time. But in the meantime, I think it's probably true that
breaking things down into independently-reviewable patches might be
helpful.

(On the other hand, there are already four separate patches for ECPG
in the queue ... does it really need to be broken down more than
that?)

regards, tom lane

On Thu, Oct 08, 2009 at 01:15:58PM +0200, Boszormenyi Zoltan wrote:
> > What's the point of that? It can't be applied without documentation,
> > and it just makes life more complicated to have two separate patch
> > files floating around.
> It's easier to write the documentation for all changes at once.
> I would have the same situation that happened with the code,
> the patches with the documentation added would strictly depend
> on each other again. Also, Michael Meskes applied the "string"
> pseudo-type patch without the documentation, despite the patch
> had it, maybe at an improper place. With a tongue-in-cheek

I don't get it. Are you blaming me for committing you patch although it had no
documentation? Or for not committing the documentation part of it? I definitely
did not remove anything on purpose. But if I missed something a short note
would have been a better way to tell me. If the first interpretation is right I
better not comment.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes(at)jabber(dot)org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Michael Meskes írta:
> On Thu, Oct 08, 2009 at 01:15:58PM +0200, Boszormenyi Zoltan wrote:
>
>>> What's the point of that? It can't be applied without documentation,
>>> and it just makes life more complicated to have two separate patch
>>> files floating around.
>>>
>> It's easier to write the documentation for all changes at once.
>> I would have the same situation that happened with the code,
>> the patches with the documentation added would strictly depend
>> on each other again. Also, Michael Meskes applied the "string"
>> pseudo-type patch without the documentation, despite the patch
>> had it, maybe at an improper place. With a tongue-in-cheek
>>
>
> I don't get it. Are you blaming me for committing you patch although it had no
> documentation?

No blaming, but sorry, it definitely had, in both these rounds
of split-up ECPG patchsets:

2009-05-15:
http://archives.postgresql.org/message-id/4A5E0F1D.7030004@cybertec.at
2009-08-03:
http://archives.postgresql.org/message-id/4A770C7B.1060404@cybertec.at

I assumed you have applied it from the second mail, this was
the last version sent for the "string" patch. It is my fault that
I haven't put it on the CommitFest page. Indeed, the version
on the CF page doesn't have documentation. But the code you
committed seems to be the one (or very close, with some
editorialization) in the second mail quoted above.

> Or for not committing the documentation part of it? I definitely
> did not remove anything on purpose.

I guessed that it was not on purpose, but I only realized
last thursday that the documentation for "string" is missing
when I wrote the docs for the other patches.

> But if I missed something a short note
> would have been a better way to tell me.

Yes, of course. Sorry.

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/

On Sat, Oct 10, 2009 at 2:09 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)at> wrote:
> Michael Meskes írta:
>> On Thu, Oct 08, 2009 at 01:15:58PM +0200, Boszormenyi Zoltan wrote:
>>
>>>> What's the point of that?  It can't be applied without documentation,
>>>> and it just makes life more complicated to have two separate patch
>>>> files floating around.
>>>>
>>> It's easier to write the documentation for all changes at once.
>>> I would have the same situation that happened with the code,
>>> the patches with the documentation added would strictly depend
>>> on each other again. Also, Michael Meskes applied the "string"
>>> pseudo-type patch without the documentation, despite the patch
>>> had it, maybe at an improper place. With a tongue-in-cheek
>>>
>>
>> I don't get it. Are you blaming me for committing you patch although it had no
>> documentation?
>
> No blaming, but sorry, it definitely had, in both these rounds
> of split-up ECPG patchsets:
>
> 2009-05-15:
> http://archives.postgresql.org/message-id/4A5E0F1D.7030004@cybertec.at
> 2009-08-03:
> http://archives.postgresql.org/message-id/4A770C7B.1060404@cybertec.at
>
> I assumed you have applied it from the second mail, this was
> the last version sent for the "string" patch. It is my fault that
> I haven't put it on the CommitFest page. Indeed, the version
> on the CF page doesn't have documentation. But the code you
> committed seems to be the one (or very close, with some
> editorialization) in the second mail quoted above.

Since it doesn't seem that any of the patches are going to get
committed RSN, I have moved all of the open ECPG patches to the next
CommitFest and given them their own section.

...Robert