Thread

CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-09-30T01:22:04Z

It's now been two weeks since we started this CommitFest, so it seems
like a good time to review where we are.  Here are my thoughts, for
what that's worth.

Our overall rate of progress is significantly slower than it was last
time around.  At a similar point in the July CommitFest, 19 patches
had been committed (not counting 3 that were committed before the
start of the CommitFest), 11 had been returned with feedback (again,
not counting 2 from before the start of the CommitFest), and 3 had
been rejected.  The corresponding numbers for this CommitFest are 8,
7, and 3, which means that the rate of returning patches with feedback
and/or rejecting them is only modestly lower, but the rate of
committing is much lower.  I'm not sure whether this is because the
patches are more complex, because the committers have been busy with
other issues, or some other reason.

We also have fewer patches than we did last time around.  I believe we
started the last CommitFest with a bit more than 75 patches (there are
fewer now, as some were moved to this CommitFest) and we started this
one with just 48.  This somewhat balances out the slower rate of
grinding through the patch queue, but I'm still a bit worried about
the rate at which we're making progress.  It would be nice to be done
on time, and I'm not sure we're going to make it.

With respect to individual patches:

- There are three ECPG patches for which it's been difficult to find a
reviewer.  It seems we don't have any reviewers familiar with ECPG.
If anyone is able to help review these, it would be much appreciated.
- There is one dblink pach left over from last CommitFest.  Joe Conway
was going to review it the weekend of July 18th-19th, but that didn't
end up happening and so that patch is still waiting.  We might be able
to find someone else to review it, but I'm not sure whether that will
help unless there is a committer other than Joe with bandwidth to do
the final review and commit.
- Hot Standby and Streaming Replication are both huge new features in
this CommitFest, and there seems to be a fair amount of activity
around both patches.  Heikki previously expressed optimism about
getting Hot Standby done this CommitFest, but I am not sure whether he
is still feeling optimistic, or what his feelings are about Streaming
Replication, which is currently waiting on Fujii Masao for a new
version.

On the whole, it seems like patch authors have done a better job than
last time of responding to feedback in a timely fashion - very little
is falling out due to submitter inattention.  That is good, although
it also means that the percentage of patches that will require
substantive action (rather than, say, summary rejection for
non-communication) is apt to be higher.  I am also generally under the
impression that we have a larger number of complex patches this time
around.  Some of that may be because much good feedback was given in
the last CommitFest, and previously half-baked ideas are coming back a
little more well done.

...Robert

Re: CommitFest 2009-09, two weeks on

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-09-30T15:19:16Z

Robert Haas wrote:
> - Hot Standby and Streaming Replication are both huge new features in
> this CommitFest, and there seems to be a fair amount of activity
> around both patches.  Heikki previously expressed optimism about
> getting Hot Standby done this CommitFest, but I am not sure whether he
> is still feeling optimistic,

There's a lot of small things that need fixing, but nothing major. I'm
not so much optimistic, but I think we should spend the extra effort
required on hot standby to force it in in this commitfest. It's a big
feature and it really could use some alpha-testing earlier rather than
later. It would also leave time for any extra features or tweaks to be
made in the later commitfests.

OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
should be returned to author at this point, I should move on to other
patches to get the commitfest closed ASAP, and continue reviewing and
polishing that right after the commitfest.

> or what his feelings are about Streaming
> Replication, which is currently waiting on Fujii Masao for a new
> version.

I'm undecided on whether walreceiver should be a subprocess of the
startup process, or of postmaster as it was submitted. I'd appreciate if
others would take a look into that too and give opinions. And then
there's the small list of things I asked Fujii-san to work on.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-30T15:36:40Z

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
> should be returned to author at this point, I should move on to other
> patches to get the commitfest closed ASAP, and continue reviewing and
> polishing that right after the commitfest.

FWIW, I'd rather you kept focusing on those two patches while other
committers work on the rest.  From what I've seen you're finding a
whole lot of broken or at least questionable stuff, and even if they're
individually minor issues, that adds up to a lot of instability.

I agree that these patches need special attention and should not be
treated exactly the same as we'd treat smaller patches.

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-09-30T16:01:32Z

On Wed, Sep 30, 2009 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
>> should be returned to author at this point, I should move on to other
>> patches to get the commitfest closed ASAP, and continue reviewing and
>> polishing that right after the commitfest.
>
> FWIW, I'd rather you kept focusing on those two patches while other
> committers work on the rest.  From what I've seen you're finding a
> whole lot of broken or at least questionable stuff, and even if they're
> individually minor issues, that adds up to a lot of instability.
>
> I agree that these patches need special attention and should not be
> treated exactly the same as we'd treat smaller patches.

I tend to agree.  I think it's reasonable for you (meaning Heikki) to
devote far more time and effort to those patches than you would to
other patches implementing more run-of-the-mill features, and it seems
like there is no shortage of things to find and fix.  I don't think
that having you take a break to work on other patches is going to be
to the overall benefit of the project (and many of the more
significant remaining patches look like they are right up Tom's alley
anyway).

That having been said, if Hot Standby is still closer to commit than
Streaming Replication, it might make sense to push Streaming
Replication off until November, or at least post-CommitFest.  Do you
have any sense of how soon you'll feel confident to commit either
patch?

...Robert

Re: CommitFest 2009-09, two weeks on

Joe Conway <mail@joeconway.com> — 2009-09-30T16:20:25Z

Robert Haas wrote:
> - There is one dblink pach left over from last CommitFest.  Joe Conway
> was going to review it the weekend of July 18th-19th, but that didn't
> end up happening and so that patch is still waiting.  We might be able
> to find someone else to review it, but I'm not sure whether that will
> help unless there is a committer other than Joe with bandwidth to do
> the final review and commit.

I will get to it before the end of this commitfest, but I have to admit
I'm not all that excited about this patch in the first place. I don't
know that I agree with the need.

Joe

Re: CommitFest 2009-09, two weeks on

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-09-30T16:24:39Z

Robert Haas wrote:
> On Wed, Sep 30, 2009 at 11:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>>> OTOH, I'd hate to hold the commitfest hostage for that. Perhaps it
>>> should be returned to author at this point, I should move on to other
>>> patches to get the commitfest closed ASAP, and continue reviewing and
>>> polishing that right after the commitfest.
>> FWIW, I'd rather you kept focusing on those two patches while other
>> committers work on the rest.  From what I've seen you're finding a
>> whole lot of broken or at least questionable stuff, and even if they're
>> individually minor issues, that adds up to a lot of instability.
>>
>> I agree that these patches need special attention and should not be
>> treated exactly the same as we'd treat smaller patches.
> 
> I tend to agree.  I think it's reasonable for you (meaning Heikki) to
> devote far more time and effort to those patches than you would to
> other patches implementing more run-of-the-mill features, and it seems
> like there is no shortage of things to find and fix.  I don't think
> that having you take a break to work on other patches is going to be
> to the overall benefit of the project (and many of the more
> significant remaining patches look like they are right up Tom's alley
> anyway).

Ok, good, I'm more than happy to continue fine-combing hot standby.

> That having been said, if Hot Standby is still closer to commit than
> Streaming Replication, it might make sense to push Streaming
> Replication off until November, or at least post-CommitFest.

Commitfest or no-commitfest, I'm planning to continue working on the
streaming replication patch in any case until it's committed.

>  Do you
> have any sense of how soon you'll feel confident to commit either
> patch?

I'm bad at estimating. Not this week for sure, and next week I'm
traveling and won't be able to spend as much time on it as I am right
now. If no new major issues are found, and all the outstanding issues
are resolved by me or Simon by then, maybe the week after that.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-30T16:27:18Z

Joe Conway <mail@joeconway.com> writes:
> Robert Haas wrote:
>> - There is one dblink pach left over from last CommitFest.  Joe Conway
>> was going to review it the weekend of July 18th-19th, but that didn't
>> end up happening and so that patch is still waiting.  We might be able
>> to find someone else to review it, but I'm not sure whether that will
>> help unless there is a committer other than Joe with bandwidth to do
>> the final review and commit.

> I will get to it before the end of this commitfest, but I have to admit
> I'm not all that excited about this patch in the first place. I don't
> know that I agree with the need.

Well, you're the dblink expert.  If you think it should be rejected
I doubt many of us will argue with you.

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-30T16:34:23Z

Robert Haas <robertmhaas@gmail.com> writes:
> ... (and many of the more
> significant remaining patches look like they are right up Tom's alley
> anyway).

FWIW, if left to my own devices I will eventually get to everything
except the dblink, ecpg, and encoding/win32 patches.  I don't intend
to touch any of those because there are other committers better
qualified to review them.  (I don't actually think we have anybody
except Michael who's really familiar with ecpg.)

However, if no other committers are working on it it's going to be
a long commitfest ...

The other problem is that most of the patches are not Ready for
Committer anyway.

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-09-30T16:51:55Z

On Wed, Sep 30, 2009 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Joe Conway <mail@joeconway.com> writes:
>> Robert Haas wrote:
>>> - There is one dblink pach left over from last CommitFest.  Joe Conway
>>> was going to review it the weekend of July 18th-19th, but that didn't
>>> end up happening and so that patch is still waiting.  We might be able
>>> to find someone else to review it, but I'm not sure whether that will
>>> help unless there is a committer other than Joe with bandwidth to do
>>> the final review and commit.
>
>> I will get to it before the end of this commitfest, but I have to admit
>> I'm not all that excited about this patch in the first place. I don't
>> know that I agree with the need.
>
> Well, you're the dblink expert.  If you think it should be rejected
> I doubt many of us will argue with you.

Yep.  CommitFest doesn't mean "commit it"; it means "decide whether to
commit it".  Things being rejected or returned with feedback for
further improvement is fine; we're just trying to avoid long periods
with no response at all.

...Robert

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-09-30T17:01:25Z

On Wed, Sep 30, 2009 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ... (and many of the more
>> significant remaining patches look like they are right up Tom's alley
>> anyway).
>
> FWIW, if left to my own devices I will eventually get to everything
> except the dblink, ecpg, and encoding/win32 patches.  I don't intend
> to touch any of those because there are other committers better
> qualified to review them.  (I don't actually think we have anybody
> except Michael who's really familiar with ecpg.)

Thanks, I think that's helpful information.

> However, if no other committers are working on it it's going to be
> a long commitfest ...

That is my concern as well.

> The other problem is that most of the patches are not Ready for
> Committer anyway.

I (and hopefully the people who agreed to help with patch-chasing) can
work on this, but given that there are 5 that are Ready for Committer
and probably as many more that are close, and further given that in
the past 7 days exactly 1 patch from the CommitFest has been
committed, I'm not sure there's a real problem here.  If you
commit/bounce all 5 of those afternoon I will spend the evening making
sure you have a few more to tackle tomorrow.

...Robert

Re: CommitFest 2009-09, two weeks on

Andrew Dunstan <andrew@dunslane.net> — 2009-09-30T17:26:43Z

Robert Haas wrote:
> On Wed, Sep 30, 2009 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>   
>> Robert Haas <robertmhaas@gmail.com> writes:
>>     
>>> ... (and many of the more
>>> significant remaining patches look like they are right up Tom's alley
>>> anyway).
>>>       
>> FWIW, if left to my own devices I will eventually get to everything
>> except the dblink, ecpg, and encoding/win32 patches.  I don't intend
>> to touch any of those because there are other committers better
>> qualified to review them.  (I don't actually think we have anybody
>> except Michael who's really familiar with ecpg.)
>>     
>
> Thanks, I think that's helpful information.
>
>   
>> However, if no other committers are working on it it's going to be
>> a long commitfest ...
>>     
>
> That is my concern as well.
>
>   

I have been (and still am somewhat) slammed, but I can probably make 
space to work on "Encoding issues in console and eventlog on win32 
<https://commitfest.postgresql.org/action/patch_view?id=148>" some time 
in the next day or three. After that, if I still have time and nobody 
else has grabbed it, I'll move on to "CREATE LIKE INCLUDING COMMENTS and 
STORAGE <https://commitfest.postgresql.org/action/patch_view?id=172>".

cheers

andrew

Re: CommitFest 2009-09, two weeks on

Joe Conway <mail@joeconway.com> — 2009-09-30T17:33:05Z

Robert Haas wrote:
> On Wed, Sep 30, 2009 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Joe Conway <mail@joeconway.com> writes:
>>> Robert Haas wrote:
>>>> - There is one dblink pach left over from last CommitFest.  Joe Conway
>>>> was going to review it the weekend of July 18th-19th, but that didn't
>>>> end up happening and so that patch is still waiting.  We might be able
>>>> to find someone else to review it, but I'm not sure whether that will
>>>> help unless there is a committer other than Joe with bandwidth to do
>>>> the final review and commit.
>>> I will get to it before the end of this commitfest, but I have to admit
>>> I'm not all that excited about this patch in the first place. I don't
>>> know that I agree with the need.
>> Well, you're the dblink expert.  If you think it should be rejected
>> I doubt many of us will argue with you.
> 
> Yep.  CommitFest doesn't mean "commit it"; it means "decide whether to
> commit it".  Things being rejected or returned with feedback for
> further improvement is fine; we're just trying to avoid long periods
> with no response at all.

The issue is not so much technical as it is philosophical.

The patch basically forces all use of libpq by dblink to be asynchronous
(internally) so that a cancel can be sensed and passed down to the
remote side and everything cleaned up. Possibly the right thing to do,
but dblink already allows the use of async queries, and the current
synchronous method uses standard libpq calls. If all of this is really
necessary, doesn't every libpq client have the same issue? If so why
have the synchronous libpq functions at all?

So while I can vet the patch technically, and spend more time
understanding the use case, and maybe explaining it better, I think
other people should weigh in on the change as it is significant and
points to other potential issues.

Joe

Re: CommitFest 2009-09, two weeks on

Magnus Hagander <magnus@hagander.net> — 2009-09-30T18:08:34Z

On Wed, Sep 30, 2009 at 18:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> ... (and many of the more
>> significant remaining patches look like they are right up Tom's alley
>> anyway).
>
> FWIW, if left to my own devices I will eventually get to everything
> except the dblink, ecpg, and encoding/win32 patches.  I don't intend
> to touch any of those because there are other committers better
> qualified to review them.  (I don't actually think we have anybody
> except Michael who's really familiar with ecpg.)

I can certainly review the win32 encoding patch, but I was rather
hoping for some comments from others on if we're interested in a win32
only solution, or if we want something more generic. Should we just go
with the win32-only one for now?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-09-30T18:11:10Z

Magnus Hagander escribió:
> On Wed, Sep 30, 2009 at 18:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> >> ... (and many of the more
> >> significant remaining patches look like they are right up Tom's alley
> >> anyway).
> >
> > FWIW, if left to my own devices I will eventually get to everything
> > except the dblink, ecpg, and encoding/win32 patches.  I don't intend
> > to touch any of those because there are other committers better
> > qualified to review them.  (I don't actually think we have anybody
> > except Michael who's really familiar with ecpg.)
> 
> I can certainly review the win32 encoding patch, but I was rather
> hoping for some comments from others on if we're interested in a win32
> only solution, or if we want something more generic. Should we just go
> with the win32-only one for now?

Just a couple of days ago a question came on the spanish list because
someone was getting mixed UTF8 and Latin1 output in a log file.  This
was in Fedora IIRC, so maybe we do want something more general.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-30T19:38:07Z

Magnus Hagander <magnus@hagander.net> writes:
> I can certainly review the win32 encoding patch, but I was rather
> hoping for some comments from others on if we're interested in a win32
> only solution, or if we want something more generic. Should we just go
> with the win32-only one for now?

That was actually the only substantive comment I had about it.  I don't
see why it's a win32-only problem or why a win32-only solution is a good
approach.

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Magnus Hagander <magnus@hagander.net> — 2009-09-30T19:45:29Z

On Wed, Sep 30, 2009 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> I can certainly review the win32 encoding patch, but I was rather
>> hoping for some comments from others on if we're interested in a win32
>> only solution, or if we want something more generic. Should we just go
>> with the win32-only one for now?
>
> That was actually the only substantive comment I had about it.  I don't
> see why it's a win32-only problem or why a win32-only solution is a good
> approach.

Yeah, that's my thought as well.

If we want a complete one, we should reject this patch and ask for one
that does that.

If we are fine with a win32 only one, I can review this one and get it in.

I'm leaning towards us wanting a general one, but I'm unsure how much
work that will take.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-30T20:52:48Z

Joe Conway <mail@joeconway.com> writes:
> The issue is not so much technical as it is philosophical.

> The patch basically forces all use of libpq by dblink to be asynchronous
> (internally) so that a cancel can be sensed and passed down to the
> remote side and everything cleaned up. Possibly the right thing to do,
> but dblink already allows the use of async queries, and the current
> synchronous method uses standard libpq calls. If all of this is really
> necessary, doesn't every libpq client have the same issue?

Well, only the ones that want to implement cancel and don't have access
to the app's own signal handling functions.  (Which suggests that a
possible answer is to allow dblink to hook into the SIGINT catcher,
but frankly hooks in that location scare me ...)

I would argue that it's not necessarily a good idea at all: one of the
typical uses for dblink is to fake "autonomous transactions", and in
that application I don't think you *want* a cancel to propagate to the
other session.  If we did put this behavior into all dblink operations,
we'd need a way to turn it off.

Since dblink_cancel_query is already available, people who do want
cancels to propagate have the ability to do that.  I'm inclined to
think that this is complexity we don't need.

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-10-01T02:00:23Z

Joe Conway <mail@joeconway.com> wrote:

> The patch basically forces all use of libpq by dblink to be asynchronous
> (internally) so that a cancel can be sensed and passed down to the
> remote side and everything cleaned up. Possibly the right thing to do,
> but dblink already allows the use of async queries, and the current
> synchronous method uses standard libpq calls.

The point is *memory leak* in dblink when a query is canceled or
become time-out. I think it is a bug, and my patch could fix it.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Re: CommitFest 2009-09, two weeks on

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-10-01T02:11:46Z

Magnus Hagander <magnus@hagander.net> wrote:

> I can certainly review the win32 encoding patch, but I was rather
> hoping for some comments from others on if we're interested in a win32
> only solution, or if we want something more generic. Should we just go
> with the win32-only one for now?

Yes, because Windows is only platform that supports UTF-16 encoding natively.
I believe my patch is the best solution for Windows even if we have another
approach for other platforms.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Re: CommitFest 2009-09, two weeks on

Magnus Hagander <magnus@hagander.net> — 2009-10-01T11:48:35Z

On Thu, Oct 1, 2009 at 04:11, Itagaki Takahiro
<itagaki.takahiro@oss.ntt.co.jp> wrote:
>
> Magnus Hagander <magnus@hagander.net> wrote:
>
>> I can certainly review the win32 encoding patch, but I was rather
>> hoping for some comments from others on if we're interested in a win32
>> only solution, or if we want something more generic. Should we just go
>> with the win32-only one for now?
>
> Yes, because Windows is only platform that supports UTF-16 encoding natively.
> I believe my patch is the best solution for Windows even if we have another
> approach for other platforms.

Actually, I think a better argument is that since Windows will *never*
accept UTF8 logging, and that's what most databases will be in, much
of this patch will be required anyway. So I should probably review and
get this part in while we think about other solutions *as well* for
other platforms.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: CommitFest 2009-09, two weeks on

Michael Meskes <meskes@postgresql.org> — 2009-10-01T12:27:26Z

On Wed, Sep 30, 2009 at 12:34:23PM -0400, Tom Lane wrote:
> qualified to review them.  (I don't actually think we have anybody
> except Michael who's really familiar with ecpg.)

I'm afraid I'm simply not able to spend much time on this in the near future as
I'm simply too busy atm. I spend some time on these the last time, but wasn't
even able to see how Zoltan changed the points we mentioned back then, but I'm
sure he has.

As already noted the patches stack on each other. There doesn't seem to be a
technical reason for this at least not for some of those dependencies. With the
first patch changing the grammar file and thus taking quite some reviewing
effort this slows things down even more because one needs more effort to review
for instance the sqlda addition, although that one seems to be quite easy to
review.

All of this is written from the top of my head, so please bear with me if I
missed any changes in the patches.

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@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-01T13:47:07Z

Michael Meskes írta:
> On Wed, Sep 30, 2009 at 12:34:23PM -0400, Tom Lane wrote:
>   
>> qualified to review them.  (I don't actually think we have anybody
>> except Michael who's really familiar with ecpg.)
>>     
>
> I'm afraid I'm simply not able to spend much time on this in the near future as
> I'm simply too busy atm. I spend some time on these the last time, but wasn't
> even able to see how Zoltan changed the points we mentioned back then, but I'm
> sure he has.
>
> As already noted the patches stack on each other. There doesn't seem to be a
> technical reason for this at least not for some of those dependencies. With the
> first patch changing the grammar file and thus taking quite some reviewing
> effort this slows things down even more because one needs more effort to review
> for instance the sqlda addition, although that one seems to be quite easy to
> review.
>   

You're not being fair with me. The dependencies are quite
technical.

First, Tom Lane suggested to unify core and ecpg FETCH
syntaxes so both will accept optional FROM/IN, which I did.
SQLDA support adds new FETCH forms (Informix-specific
ones) so naturally these patches clash. There's no simple way
to make they separately applicable. With the first version,
the same technical dependency were also there, because of
the (already explained) grammar problem, I got 2 shift/reduce
problems in the FETCH/MOVE stmts unless I de-factorized
FORWARD and BACKWARD out of fetch_direction.
The new FETCH forms with SQLDA touched the same areas
in ecpg.addon.

Second, DESCRIBE support and SQLDA support also overlap,
because SQLDA is a new descriptor form, and DESCRIBE has to
support both SQL descriptors and SQLDA. Well, I can split the
DESCRIBE patch in half, so it will be usable on SQL descriptors
but the other part would depend on both SQLDA and basic
DESCRIBE support. Technical dependency again.

What non-technical dependencies are you talking about?
Please explain, so I may fix them. Saying it vaguely doesn't help.

When I first posted the split patchset, you didn't tell me that
the split is no good. I tried everything help I could to explain
why I did what.

Also, the current reviewer (Dan Colish) haven't contacted me despite
I offered help privately. I can't review my own patches, that's clear.
But I can't do anything else to speed review up but to offer my help
and wait for the help/explanation request that didn't arrive.

Also, I have sent some independent very small patches, that don't
even need much review. One of them (the typo in pgc.l) was already
applied and I thank you Michael, but the memory leak fix for two
improperly freed numerics is still behind. Please, look at that patch,
it should be really obvious.

> All of this is written from the top of my head, so please bear with me if I
> missed any changes in the patches.
>
> Michael
>   

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/

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-10-01T15:02:03Z

Boszormenyi Zoltan escribió:

> First, Tom Lane suggested to unify core and ecpg FETCH
> syntaxes so both will accept optional FROM/IN, which I did.
> SQLDA support adds new FETCH forms (Informix-specific
> ones) so naturally these patches clash. There's no simple way
> to make they separately applicable. With the first version,
> the same technical dependency were also there, because of
> the (already explained) grammar problem, I got 2 shift/reduce
> problems in the FETCH/MOVE stmts unless I de-factorized
> FORWARD and BACKWARD out of fetch_direction.
> The new FETCH forms with SQLDA touched the same areas
> in ecpg.addon.

Probably the parts that touch the core grammar can be reviewed and
applied separately.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: CommitFest 2009-09, two weeks on

Michael Meskes <meskes@postgresql.org> — 2009-10-01T16:12:18Z

On Thu, Oct 01, 2009 at 03:47:07PM +0200, Boszormenyi Zoltan wrote:
> You're not being fair with me. The dependencies are quite
> technical.

I'm sorry that you interpreted my email this way, it wasn't at all meant to
offend you.

> First, Tom Lane suggested to unify core and ecpg FETCH
> syntaxes so both will accept optional FROM/IN, which I did.
> SQLDA support adds new FETCH forms (Informix-specific

This is actually one I was talking about. Adding SQLDA *only* seems like an
easy one, as soon as it also hits the parser it becomes more complicated. This
is not to say that it is not doable, but it wasn't for me with my time
constraints. I was just explaining why I didn't delve into these any more so
far.

> When I first posted the split patchset, you didn't tell me that
> the split is no good. I tried everything help I could to explain
> why I did what.

Well actually I did, but that's not the problem here. I have no idea why you
are ranting like this just because of me excusing myself for a lack of time.

> Also, the current reviewer (Dan Colish) haven't contacted me despite
> I offered help privately. I can't review my own patches, that's clear.
> But I can't do anything else to speed review up but to offer my help
> and wait for the help/explanation request that didn't arrive.

Okay, so it's all my fault. Do you feel better now? 

> Also, I have sent some independent very small patches, that don't
> even need much review. One of them (the typo in pgc.l) was already
> applied and I thank you Michael, but the memory leak fix for two
> improperly freed numerics is still behind. Please, look at that patch,
> it should be really obvious.

My last commit was one of your patches, obviously I didn't do anything on ecpg
since. I'm not sure what you are trying to tell me, you might want to explain
the last two sentences. But keep in mind you are *not* the one to decide how I
spend my spare time.

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@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-01T17:21:54Z

Michael Meskes írta:
> On Thu, Oct 01, 2009 at 03:47:07PM +0200, Boszormenyi Zoltan wrote:
>   
>> You're not being fair with me. The dependencies are quite
>> technical.
>>     
>
> I'm sorry that you interpreted my email this way, it wasn't at all meant to
> offend you.
>   

Please, accept my apologies, I only tried to express my
frustration, this is not a good situation for either of us.
You were busy with your job and other occupations.
We have a serious project going on that depend on
ECPG being more compatible to Informix.

>> First, Tom Lane suggested to unify core and ecpg FETCH
>> syntaxes so both will accept optional FROM/IN, which I did.
>> SQLDA support adds new FETCH forms (Informix-specific
>>     
>
> This is actually one I was talking about. Adding SQLDA *only* seems like an
> easy one, as soon as it also hits the parser it becomes more complicated. This
> is not to say that it is not doable, but it wasn't for me with my time
> constraints. I was just explaining why I didn't delve into these any more so
> far.
>   

This is now clear, I will separate that part out and post it.
But this means that the same core grammar parts will be touched
twice because two basic things are done there:
- optional FROM/IN in core grammar, ECPG grammar updated
- "name" -> "cursor_name" transition in cursor-related statements
  in the core grammar and the dynamic cursorname patch
So, technically dependant patches again, but maybe easier on the reviewer.
Would this be accepted this way? Or the two modification washed into one?

>> When I first posted the split patchset, you didn't tell me that
>> the split is no good. I tried everything help I could to explain
>> why I did what.
>>     
>
> Well actually I did, but that's not the problem here. I have no idea why you
> are ranting like this just because of me excusing myself for a lack of time.
>   

I am very sorry, under stress sometimes I get like a steam
engine overloaded. Not a good behaviour at 37, I know.
I am very sorry.

>> Also, the current reviewer (Dan Colish) haven't contacted me despite
>> I offered help privately. I can't review my own patches, that's clear.
>> But I can't do anything else to speed review up but to offer my help
>> and wait for the help/explanation request that didn't arrive.
>>     
>
> Okay, so it's all my fault. Do you feel better now? 
>   

It's not your fault at all, it's a difficult situation and I had to
express it somehow. My tone was not proper.

BTW, thanks for adding my small collected knowledge
about the ECPG grammar as official documentation.

>> Also, I have sent some independent very small patches, that don't
>> even need much review. One of them (the typo in pgc.l) was already
>> applied and I thank you Michael, but the memory leak fix for two
>> improperly freed numerics is still behind. Please, look at that patch,
>> it should be really obvious.
>>     
>
> My last commit was one of your patches,

Yes, the pgc.l one-liner. Thanks again for committing that.

>  obviously I didn't do anything on ecpg
> since. I'm not sure what you are trying to tell me, you might want to explain
> the last two sentences.

I thought you forgot that patch, the "please, look at that patch"
was an (I thought) polite request, it's really two one-liners.

>  But keep in mind you are *not* the one to decide how I
> spend my spare time.
>   

Obviously. Please, accept my apologies.

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/

Re: CommitFest 2009-09, two weeks on

Michael Meskes <meskes@postgresql.org> — 2009-10-01T18:11:32Z

On Thu, Oct 01, 2009 at 07:21:54PM +0200, Boszormenyi Zoltan wrote:
> Please, accept my apologies, I only tried to express my
> frustration, this is not a good situation for either of us.

Apologies accepted, email is a difficult means of communication anyway. It
leads to misunderstanding IMO.

> You were busy with your job and other occupations.
> We have a serious project going on that depend on
> ECPG being more compatible to Informix.

Please keep in mind that the needs of your business project cannot and will not
influence the way PostgreSQL as on OSS project will work.

> Would this be accepted this way? Or the two modification washed into one?

It is accepted either way. I was just pointing out that it might be easier to
review/commit at least parts of your patches if they can be applied seperately.

> I thought you forgot that patch, the "please, look at that patch"
> was an (I thought) polite request, it's really two one-liners.

Well, this is true as the patch was buried in the long thread containing all
the other ones. And yes, now that you brought it into my memory again, I
already committed it. Sorry for missing it.

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@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-01T19:05:55Z

Michael Meskes írta:
> On Thu, Oct 01, 2009 at 07:21:54PM +0200, Boszormenyi Zoltan wrote:
>   
>> Please, accept my apologies, I only tried to express my
>> frustration, this is not a good situation for either of us.
>>     
>
> Apologies accepted, email is a difficult means of communication anyway. It
> leads to misunderstanding IMO.
>
>   
>> You were busy with your job and other occupations.
>> We have a serious project going on that depend on
>> ECPG being more compatible to Informix.
>>     
>
> Please keep in mind that the needs of your business project cannot and will not
> influence the way PostgreSQL as on OSS project will work.
>   

Yes, but technical problems and solutions do. ECPG claims
to be ESQL/C compatible, but at places it's only half compatible.
For our project to succeed, we need more compatibility in ECPG.
It's easier to solve these problems in ECPG than to code around it
in literally thousands of little programs.

BTW, a thought about the comment in ecpg.header about adjust_informix():

        /* Informix accepts DECLARE with variables that are out of scope
when OPEN is called.
         * for instance you can declare variables in a function, and
then subsequently use them
         * {
         *      declare_vars();
         *      exec sql ... which uses vars declared in the above function
         *
         * This breaks standard and leads to some very dangerous
programming.

This comment is misleading and reflects quite a narrow POV.
Not only OPEN and DECLARE may be out of scope,
but FETCH and CLOSE as well. The reason why ESQL/C
allows this construct is that this ultimately allows using
embedded SQL in event-driven code in a straightforward way.
For this purpose, native ECPG code is not usable currently,
or you need programming tricks, like tracking whether the
cursor is open and protecting DECLARE and OPEN under
some conditional branch to avoid double open, etc. A straight
DECLARE, OPEN, FETCH(s) and CLOSE series in
the same function is only good for batch programming.

>> Would this be accepted this way? Or the two modification washed into one?
>>     
>
> It is accepted either way. I was just pointing out that it might be easier to
> review/commit at least parts of your patches if they can be applied seperately.
>   

Okay, I will split the remaining patches into more little pieces
that can reviewed more easily. Some patches will still build
on earlier ones in the series, that's unavoidable.

>> I thought you forgot that patch, the "please, look at that patch"
>> was an (I thought) polite request, it's really two one-liners.
>>     
>
> Well, this is true as the patch was buried in the long thread containing all
> the other ones. And yes, now that you brought it into my memory again, I
> already committed it. Sorry for missing it.
>   

Thank you very much for committing it.

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/

Re: CommitFest 2009-09, two weeks on

Michael Meskes <meskes@postgresql.org> — 2009-10-02T13:37:58Z

On Thu, Oct 01, 2009 at 09:05:55PM +0200, Boszormenyi Zoltan wrote:
> Yes, but technical problems and solutions do. ECPG claims
> to be ESQL/C compatible, but at places it's only half compatible.

Where does it claim to be fully compatible?

> This comment is misleading and reflects quite a narrow POV.
> Not only OPEN and DECLARE may be out of scope,
> but FETCH and CLOSE as well. The reason why ESQL/C
> allows this construct is that this ultimately allows using
> embedded SQL in event-driven code in a straightforward way.
> For this purpose, native ECPG code is not usable currently,
> or you need programming tricks, like tracking whether the
> cursor is open and protecting DECLARE and OPEN under
> some conditional branch to avoid double open, etc. A straight
> DECLARE, OPEN, FETCH(s) and CLOSE series in
> the same function is only good for batch programming.

Examples?

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@jabber.org
Go VfL Borussia! Go SF 49ers! Use Debian GNU/Linux! Use PostgreSQL!

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-02T14:27:20Z

Michael Meskes írta:
> On Thu, Oct 01, 2009 at 09:05:55PM +0200, Boszormenyi Zoltan wrote:
>   
>> Yes, but technical problems and solutions do. ECPG claims
>> to be ESQL/C compatible, but at places it's only half compatible.
>>     
>
> Where does it claim to be fully compatible?
>   

I didn't say it claims to be fully compatible, but it's
a "hint" that "ecpg -C INFORMIX[_SE]" exists. :-)

>> This comment is misleading and reflects quite a narrow POV.
>> Not only OPEN and DECLARE may be out of scope,
>> but FETCH and CLOSE as well. The reason why ESQL/C
>> allows this construct is that this ultimately allows using
>> embedded SQL in event-driven code in a straightforward way.
>> For this purpose, native ECPG code is not usable currently,
>> or you need programming tricks, like tracking whether the
>> cursor is open and protecting DECLARE and OPEN under
>> some conditional branch to avoid double open, etc. A straight
>> DECLARE, OPEN, FETCH(s) and CLOSE series in
>> the same function is only good for batch programming.
>>     
>
> Examples?
>   

I took my outofscope.pgc example from our "out of
scope" patch and shortened it. Compare the ecpg-native and
compat outputs and the esql output of the same file. The ecpg
outputs are generated with 8.4.1 plus out patches added, the
native output differs only from 8.3.7's ecpg in the amount of
whitespaces emitted between literals. The get_record1()
function can be called from a button-handler, e.g. when pressing
PgDn, or similar... No tricks needed, straightforward code.

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/

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-03T01:01:28Z

Hi,

Michael Meskes írta:
> It is accepted either way. I was just pointing out that it might be easier to
> review/commit at least parts of your patches if they can be applied seperately.
>   

I have split up (and cleaned up a little) the dynamic
cursorname patch into smaller logical, easier-to-review
pieces. Descriptions below.

1) 1a-unified-optfromin-fetch-ctxdiff.patch

ecpg supports optional FROM/IN in FETCH and
MOVE statements (mainly because it's required by
Informix-compatibility). Unify core and ecpg grammar
as per Tom Lane's suggestion.

2) 1b-cursor_name-ctxdiff.patch

"name" -> "cursor_name" transition in core grammar
and ecpg grammar. Currently it does nothing, it's a
preparation phase. Depends on patch 1.

3) 1c-remove-var-from-list.patch

Introduce function remove_variable_from_list().
It is used by the dynamic cursor, SQLDA and DESCRIBE
patches for different reasons. Applicable separately.

4) 1d-dynamiccur-ctxdiff.patch

The real point of the whole series in this email.
Extend "cursor_name" in the ecpg grammar to actually
accept a character variable. The cursorname-as-variable
is replaced in the final SQL script with a $0 placeholder.
Doesn't compile as-is, requires patch 5 to get the
two shift/reduce conflicts fixed. Depends on patches
1, 2 and 3.

5) 1e-fix-shiftreduce-ctxdiff.patch

De-factorize "BACKWARD opt_from_in cursor_name"
and "FORWARD opt_from_in cursor_name" out of
fetch_args and pull them up into FetchStmt in the ecpg
grammar. Depends on patch 4.
One line in parse.pl is not clear for me:
    $replace_line{'fetch_args'} = 'ignore';
The genarated preproc.y is the same with or without
this line. But as the previous version had it with
"fetch_direction", I left it in.

6) 1f-cursorname-in-varchar-ctxdiff.patch

Allow that varchar can be used as cursorname as well.
Other character variable types were already supported.
Depends on patch 4.

7) 1g-regressiontests-ctxdiff.patch

Introduce cursor.pgc regression test for both native
and compat mode. Depends on all patches.

8) 1h-fix-parse.pl-ctxdiff.patch

Now useless patch, in the previous dynamic cursorname
patch the following scenario occured: the same rule
had both an "addon" and a "rule" extension. Without
this fix, the following code was generated in preproc.y:
    ruleA: <accepted syntax>
       {
             <addon code block>
       {
             <automatic code block>
       }
With the cleanup I did during this splitup, this scenario
doesn't happen, but this fix may be considered useful.
Applicable separately.

After every patch (except 4) both the core and ecpg
"make check" are successful.

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/

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-10-03T01:03:23Z

On Fri, Oct 2, 2009 at 9:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> Hi,
>
> Michael Meskes írta:
>> It is accepted either way. I was just pointing out that it might be easier to
>> review/commit at least parts of your patches if they can be applied seperately.
>>
>
> I have split up (and cleaned up a little) the dynamic
> cursorname patch into smaller logical, easier-to-review
> pieces. Descriptions below.
>
> 1) 1a-unified-optfromin-fetch-ctxdiff.patch
>
> ecpg supports optional FROM/IN in FETCH and
> MOVE statements (mainly because it's required by
> Informix-compatibility). Unify core and ecpg grammar
> as per Tom Lane's suggestion.
>
> 2) 1b-cursor_name-ctxdiff.patch
>
> "name" -> "cursor_name" transition in core grammar
> and ecpg grammar. Currently it does nothing, it's a
> preparation phase. Depends on patch 1.
>
> 3) 1c-remove-var-from-list.patch
>
> Introduce function remove_variable_from_list().
> It is used by the dynamic cursor, SQLDA and DESCRIBE
> patches for different reasons. Applicable separately.
>
> 4) 1d-dynamiccur-ctxdiff.patch
>
> The real point of the whole series in this email.
> Extend "cursor_name" in the ecpg grammar to actually
> accept a character variable. The cursorname-as-variable
> is replaced in the final SQL script with a $0 placeholder.
> Doesn't compile as-is, requires patch 5 to get the
> two shift/reduce conflicts fixed. Depends on patches
> 1, 2 and 3.
>
> 5) 1e-fix-shiftreduce-ctxdiff.patch
>
> De-factorize "BACKWARD opt_from_in cursor_name"
> and "FORWARD opt_from_in cursor_name" out of
> fetch_args and pull them up into FetchStmt in the ecpg
> grammar. Depends on patch 4.
> One line in parse.pl is not clear for me:
>    $replace_line{'fetch_args'} = 'ignore';
> The genarated preproc.y is the same with or without
> this line. But as the previous version had it with
> "fetch_direction", I left it in.
>
> 6) 1f-cursorname-in-varchar-ctxdiff.patch
>
> Allow that varchar can be used as cursorname as well.
> Other character variable types were already supported.
> Depends on patch 4.
>
> 7) 1g-regressiontests-ctxdiff.patch
>
> Introduce cursor.pgc regression test for both native
> and compat mode. Depends on all patches.
>
> 8) 1h-fix-parse.pl-ctxdiff.patch
>
> Now useless patch, in the previous dynamic cursorname
> patch the following scenario occured: the same rule
> had both an "addon" and a "rule" extension. Without
> this fix, the following code was generated in preproc.y:
>    ruleA: <accepted syntax>
>       {
>             <addon code block>
>       {
>             <automatic code block>
>       }
> With the cleanup I did during this splitup, this scenario
> doesn't happen, but this fix may be considered useful.
> Applicable separately.
>
> After every patch (except 4) both the core and ecpg
> "make check" are successful.

It would've been nice if you'd changed the subject line before posting these.

Also, please update commitfest.postgresql.org appropriately.

...Robert

ECPG dynamic cursorname patch revised and split Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-10-03T05:24:32Z

Robert Haas írta:
> On Fri, Oct 2, 2009 at 9:01 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> Hi,
>>
>> Michael Meskes írta:
>>     
>>> It is accepted either way. I was just pointing out that it might be easier to
>>> review/commit at least parts of your patches if they can be applied seperately.
>>>
>>>       
>> I have split up (and cleaned up a little) the dynamic
>> cursorname patch into smaller logical, easier-to-review
>> pieces. Descriptions below.
>>
>> ...
>> After every patch (except 4) both the core and ecpg
>> "make check" are successful.
>>     
>
> It would've been nice if you'd changed the subject line before posting these.
>   

At 3am, I forgot this. :(
I hope the archive threading doesn't break and
my previous mail will be referenced by this one.

> Also, please update commitfest.postgresql.org appropriately.
>   

I just did, thanks for the reminder.

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/

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-10-04T00:54:50Z

On Thu, Oct 1, 2009 at 7:48 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Thu, Oct 1, 2009 at 04:11, Itagaki Takahiro
> <itagaki.takahiro@oss.ntt.co.jp> wrote:
>>
>> Magnus Hagander <magnus@hagander.net> wrote:
>>
>>> I can certainly review the win32 encoding patch, but I was rather
>>> hoping for some comments from others on if we're interested in a win32
>>> only solution, or if we want something more generic. Should we just go
>>> with the win32-only one for now?
>>
>> Yes, because Windows is only platform that supports UTF-16 encoding natively.
>> I believe my patch is the best solution for Windows even if we have another
>> approach for other platforms.
>
> Actually, I think a better argument is that since Windows will *never*
> accept UTF8 logging, and that's what most databases will be in, much
> of this patch will be required anyway. So I should probably review and
> get this part in while we think about other solutions *as well* for
> other platforms.

Given the above, it seems that perhaps we could go ahead and apply this?

...Robert

Re: CommitFest 2009-09, two weeks on

Joe Conway <mail@joeconway.com> — 2009-10-04T02:47:32Z

Itagaki Takahiro wrote:
> The point is *memory leak* in dblink when a query is canceled or
> become time-out. I think it is a bug, and my patch could fix it.

Please see if this works for you.

Joe

dblink memory leak

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-10-05T02:46:45Z

Joe Conway <mail@joeconway.com> wrote:

> > The point is *memory leak* in dblink when a query is canceled or
> > become time-out. I think it is a bug, and my patch could fix it.
> 
> Please see if this works for you.

It does not work because errors can occur in caller of dblink functions;
Error callback should be still registered after SRF_RETURN_NEXT, so we
cannot place callback context on stack of the function. More works needed.

RegisterExprContextCallback() might be good for this purpose,
but we need to modify callbacks are fired even if error.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Re: dblink memory leak

Tom Lane <tgl@sss.pgh.pa.us> — 2009-10-05T02:53:02Z

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Joe Conway <mail@joeconway.com> wrote:
>> Please see if this works for you.

> It does not work because errors can occur in caller of dblink functions;
> Error callback should be still registered after SRF_RETURN_NEXT, so we
> cannot place callback context on stack of the function. More works needed.

Yeah, I meant to comment on that: it's an abuse of the error context
mechanism and will never be safe.  (An example is that if someone
innocently did an elog(LOG) or something like that, the callback would
get triggered.)  The global PGresult value seems awfully dangerous too.

I think what you want to do instead is use PG_TRY blocks to ensure that
transient results are cleaned up.

			regards, tom lane

Re: dblink memory leak

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-10-05T03:12:55Z

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I think what you want to do instead is use PG_TRY blocks to ensure that
> transient results are cleaned up.

I think PG_TRY blocks are not enough, too. PG_TRY requires a statement
block, but we need to return from dblink functions per tuple.
Error and interruption can occur at the caller:

    ExecMakeTableFunctionResult()
    {
        for (;;)
        {
*here*      CHECK_FOR_INTERRUPTS();
            result = FunctionCallInvoke(&fcinfo);  => { PG_TRY ... END }
            if (rsinfo.isDone == ExprEndResult)
                break;
            tuplestore_puttuple(tupstore, &tmptup);
        }
    }

Also, we should think SRF-functions might not be called repeatedly
until max_calls whether the transaction is committed or rollbacked
because we might have some optimization in FunctionScan in the future.
For example:
    SELECT * FROM dblink() LIMIT 3
might call dblink() only 3 times if we optimize executor logic
(it should not occur for now, though).

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Re: dblink memory leak

Tom Lane <tgl@sss.pgh.pa.us> — 2009-10-05T03:16:33Z

Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> I think PG_TRY blocks are not enough, too. PG_TRY requires a statement
> block, but we need to return from dblink functions per tuple.

That bit will have to be undone.  There is no reason for dblink not to
return a tuplestore.

			regards, tom lane

Re: dblink memory leak

Joe Conway <mail@joeconway.com> — 2009-10-05T17:46:03Z

Tom Lane wrote:
> Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
>> I think PG_TRY blocks are not enough, too. PG_TRY requires a statement
>> block, but we need to return from dblink functions per tuple.
> 
> That bit will have to be undone.  There is no reason for dblink not to
> return a tuplestore.

That's a really good point. It was originally written thinking we would
eventually be able to stream tuples rather than materialize them, but
given that many years have passed and we are no closer (I believe) to a
true streaming mode for SRFs, a tuplestore would be much cleaner.

Given that change, is there even any leak to even worry about? As long
as the PGresult object is created in the correct memory context, it
ought to get cleaned up automatically, no?

I can't promise to make this change before 15 October, but I will get to
it before the end of CF3.

Joe

Re: dblink memory leak

Tom Lane <tgl@sss.pgh.pa.us> — 2009-10-05T18:53:24Z

Joe Conway <mail@joeconway.com> writes:
> Given that change, is there even any leak to even worry about? As long
> as the PGresult object is created in the correct memory context, it
> ought to get cleaned up automatically, no?

No, because libpq knows nothing of backend memory contexts; it just
allocates with malloc.  You'll still need a PG_TRY block to ensure you
release PGresults during error cleanup.  The change to using tuplestores
will just help you localize that requirement in well-defined places.

> I can't promise to make this change before 15 October, but I will get to
> it before the end of CF3.

No big hurry, I think, considering the leak has always been there.

			regards, tom lane

Re: dblink memory leak

Joe Conway <mail@joeconway.com> — 2009-10-05T18:59:11Z

Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>> Given that change, is there even any leak to even worry about? As long
>> as the PGresult object is created in the correct memory context, it
>> ought to get cleaned up automatically, no?
> 
> No, because libpq knows nothing of backend memory contexts; it just
> allocates with malloc.  You'll still need a PG_TRY block to ensure you
> release PGresults during error cleanup.  The change to using tuplestores
> will just help you localize that requirement in well-defined places.

I should have known that! Thanks for the wack on the head...

>> I can't promise to make this change before 15 October, but I will get to
>> it before the end of CF3.
> 
> No big hurry, I think, considering the leak has always been there.

Great. It seems like this is too invasive a change to backport. My
feeling is that not enough people have complained about this specific
scenario to warrant the risk.

Joe

Re: dblink memory leak

Tom Lane <tgl@sss.pgh.pa.us> — 2009-10-05T19:41:23Z

Joe Conway <mail@joeconway.com> writes:
> Tom Lane wrote:
>> No big hurry, I think, considering the leak has always been there.

> Great. It seems like this is too invasive a change to backport. My
> feeling is that not enough people have complained about this specific
> scenario to warrant the risk.

Agreed, the risk/reward ratio doesn't seem favorable for a backport.

			regards, tom lane

Re: dblink memory leak

Robert Haas <robertmhaas@gmail.com> — 2009-10-06T00:48:33Z

On Mon, Oct 5, 2009 at 1:46 PM, Joe Conway <mail@joeconway.com> wrote:
> Tom Lane wrote:
>> Itagaki Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
>>> I think PG_TRY blocks are not enough, too. PG_TRY requires a statement
>>> block, but we need to return from dblink functions per tuple.
>>
>> That bit will have to be undone.  There is no reason for dblink not to
>> return a tuplestore.
>
> That's a really good point. It was originally written thinking we would
> eventually be able to stream tuples rather than materialize them, but
> given that many years have passed and we are no closer (I believe) to a
> true streaming mode for SRFs, a tuplestore would be much cleaner.
>
> Given that change, is there even any leak to even worry about? As long
> as the PGresult object is created in the correct memory context, it
> ought to get cleaned up automatically, no?
>
> I can't promise to make this change before 15 October, but I will get to
> it before the end of CF3.

Another possibility is that Itagaki Takahiro, who developed the
original patch, might be willing to develop something along these
lines instead.

However, it does seem that that original patch will not be accepted,
for the reason that the committers prefer the approach discussed
upthread.  Therefore, I am marking the "query cancel issues in dblink"
patch as "Returned with Feedback".

...Robert

Re: dblink memory leak

Joe Conway <mail@joeconway.com> — 2009-10-06T01:14:36Z

Robert Haas wrote:
> On Mon, Oct 5, 2009 at 1:46 PM, Joe Conway <mail@joeconway.com> wrote:
>> I can't promise to make this change before 15 October, but I will get to
>> it before the end of CF3.
> 
> Another possibility is that Itagaki Takahiro, who developed the
> original patch, might be willing to develop something along these
> lines instead.

Either way is fine -- good suggestion ;-)

> However, it does seem that that original patch will not be accepted,
> for the reason that the committers prefer the approach discussed
> upthread.  Therefore, I am marking the "query cancel issues in dblink"
> patch as "Returned with Feedback".

Yes, thanks for doing that.

Joe

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-10-15T01:20:23Z

On Wed, Sep 30, 2009 at 12:24 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
>>  Do you
>> have any sense of how soon you'll feel confident to commit either
>> patch?
>
> I'm bad at estimating. Not this week for sure, and next week I'm
> traveling and won't be able to spend as much time on it as I am right
> now. If no new major issues are found, and all the outstanding issues
> are resolved by me or Simon by then, maybe the week after that.

It's now the week after that, so unless you're in the process of
typing that 'cvs commit' command, I'm going to move HS + SR out to the
next CommitFest so that we can close this one out and stamp alpha2.

<brief pause for objections will now ensue>

...Robert

Re: CommitFest 2009-09, two weeks on

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2009-10-15T01:35:18Z

Robert Haas wrote:
> On Wed, Sep 30, 2009 at 12:24 PM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>>>  Do you
>>> have any sense of how soon you'll feel confident to commit either
>>> patch?
>> I'm bad at estimating. Not this week for sure, and next week I'm
>> traveling and won't be able to spend as much time on it as I am right
>> now. If no new major issues are found, and all the outstanding issues
>> are resolved by me or Simon by then, maybe the week after that.
> 
> It's now the week after that, so unless you're in the process of
> typing that 'cvs commit' command, I'm going to move HS + SR out to the
> next CommitFest so that we can close this one out and stamp alpha2.
> 
> <brief pause for objections will now ensue>

I have a question.
When we register the postponed patches on the CF-Nov site again,
which tag should be choosen? "Needs Review"? or "Ready for Commiter"?

Thanks,
-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-10-15T01:45:03Z

2009/10/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> Robert Haas wrote:
>> On Wed, Sep 30, 2009 at 12:24 PM, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>>>  Do you
>>>> have any sense of how soon you'll feel confident to commit either
>>>> patch?
>>> I'm bad at estimating. Not this week for sure, and next week I'm
>>> traveling and won't be able to spend as much time on it as I am right
>>> now. If no new major issues are found, and all the outstanding issues
>>> are resolved by me or Simon by then, maybe the week after that.
>>
>> It's now the week after that, so unless you're in the process of
>> typing that 'cvs commit' command, I'm going to move HS + SR out to the
>> next CommitFest so that we can close this one out and stamp alpha2.
>>
>> <brief pause for objections will now ensue>
>
> I have a question.
> When we register the postponed patches on the CF-Nov site again,
> which tag should be choosen? "Needs Review"? or "Ready for Commiter"?

Needs Review.

...Robert

Re: CommitFest 2009-09, two weeks on

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-10-15T06:27:05Z

Robert Haas wrote:
> On Wed, Sep 30, 2009 at 12:24 PM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>>>  Do you
>>> have any sense of how soon you'll feel confident to commit either
>>> patch?
>> I'm bad at estimating. Not this week for sure, and next week I'm
>> traveling and won't be able to spend as much time on it as I am right
>> now. If no new major issues are found, and all the outstanding issues
>> are resolved by me or Simon by then, maybe the week after that.
> 
> It's now the week after that, so unless you're in the process of
> typing that 'cvs commit' command, I'm going to move HS + SR out to the
> next CommitFest so that we can close this one out and stamp alpha2.

No objections here.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-10-16T01:22:47Z

On Thu, Oct 15, 2009 at 2:27 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Robert Haas wrote:
>> On Wed, Sep 30, 2009 at 12:24 PM, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>>>  Do you
>>>> have any sense of how soon you'll feel confident to commit either
>>>> patch?
>>> I'm bad at estimating. Not this week for sure, and next week I'm
>>> traveling and won't be able to spend as much time on it as I am right
>>> now. If no new major issues are found, and all the outstanding issues
>>> are resolved by me or Simon by then, maybe the week after that.
>>
>> It's now the week after that, so unless you're in the process of
>> typing that 'cvs commit' command, I'm going to move HS + SR out to the
>> next CommitFest so that we can close this one out and stamp alpha2.
>
> No objections here.

OK, done.

...Robert

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-11T19:27:34Z

Boszormenyi Zoltan escribió:

> I have split up (and cleaned up a little) the dynamic
> cursorname patch into smaller logical, easier-to-review
> pieces. Descriptions below.
> 
> 1) 1a-unified-optfromin-fetch-ctxdiff.patch
> 
> ecpg supports optional FROM/IN in FETCH and
> MOVE statements (mainly because it's required by
> Informix-compatibility). Unify core and ecpg grammar
> as per Tom Lane's suggestion.

I have applied this patch after some tinkering.  I mainly added support
for "fetch_args: FORWARD opt_from_in name" and "BACKWARD opt_from_in
name" in ecpg.addons which apparently you forgot.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: CommitFest 2009-09, two weeks on

Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-11T19:32:51Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> I have applied this patch after some tinkering.

Shouldn't there be a docs change in that commit?

			regards, tom lane

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-11T20:08:33Z

Tom Lane escribió:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > I have applied this patch after some tinkering.
> 
> Shouldn't there be a docs change in that commit?

True -- fixed.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-11T20:32:01Z

Boszormenyi Zoltan escribió:

> 2) 1b-cursor_name-ctxdiff.patch
> 
> "name" -> "cursor_name" transition in core grammar
> and ecpg grammar. Currently it does nothing, it's a
> preparation phase. Depends on patch 1.

Applied this part too.

I cannot apply the other ones -- they belong to the ECPG maintainer.  I
hope I cleared his road a bit.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-12T10:12:29Z

Alvaro Herrera írta:
> Boszormenyi Zoltan escribió:
>
>   
>> I have split up (and cleaned up a little) the dynamic
>> cursorname patch into smaller logical, easier-to-review
>> pieces. Descriptions below.
>>
>> 1) 1a-unified-optfromin-fetch-ctxdiff.patch
>>
>> ecpg supports optional FROM/IN in FETCH and
>> MOVE statements (mainly because it's required by
>> Informix-compatibility). Unify core and ecpg grammar
>> as per Tom Lane's suggestion.
>>     
>
> I have applied this patch after some tinkering.  I mainly added support
> for "fetch_args: FORWARD opt_from_in name" and "BACKWARD opt_from_in
> name" in ecpg.addons which apparently you forgot.
>   

Thanks. Your fix is correct if this patch is considered
standalone. This means I have to re-post the later
patches to fix the reject this fix causes in them.

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/

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-12T10:17:53Z

Alvaro Herrera írta:
> Boszormenyi Zoltan escribió:
>
>   
>> 2) 1b-cursor_name-ctxdiff.patch
>>
>> "name" -> "cursor_name" transition in core grammar
>> and ecpg grammar. Currently it does nothing, it's a
>> preparation phase. Depends on patch 1.
>>     
>
> Applied this part too.
>
> I cannot apply the other ones -- they belong to the ECPG maintainer.  I
> hope I cleared his road a bit.
>   

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/

Re: CommitFest 2009-09, two weeks on

Alvaro Herrera <alvherre@commandprompt.com> — 2009-11-12T19:49:39Z

Boszormenyi Zoltan escribió:
> Alvaro Herrera írta:

> > I have applied this patch after some tinkering.  I mainly added support
> > for "fetch_args: FORWARD opt_from_in name" and "BACKWARD opt_from_in
> > name" in ecpg.addons which apparently you forgot.
> 
> Thanks. Your fix is correct if this patch is considered
> standalone. This means I have to re-post the later
> patches to fix the reject this fix causes in them.

Yeah.  BTW I don't think the rest of the pieces in this series make
sense to apply separately, because they don't do anything useful by
themselves (one of them introduces an unused function, what good is in
that?).  I think they should be submitted as a single patch.

If you want to submit patches in a series like this one, they need to be
considered standalone, I think.  The Linux kernel devs work differently
than us here.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-11-12T20:07:27Z

On Thu, Nov 12, 2009 at 2:49 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Boszormenyi Zoltan escribió:
>> Alvaro Herrera írta:
>
>> > I have applied this patch after some tinkering.  I mainly added support
>> > for "fetch_args: FORWARD opt_from_in name" and "BACKWARD opt_from_in
>> > name" in ecpg.addons which apparently you forgot.
>>
>> Thanks. Your fix is correct if this patch is considered
>> standalone. This means I have to re-post the later
>> patches to fix the reject this fix causes in them.
>
> Yeah.  BTW I don't think the rest of the pieces in this series make
> sense to apply separately, because they don't do anything useful by
> themselves (one of them introduces an unused function, what good is in
> that?).  I think they should be submitted as a single patch.
>
> If you want to submit patches in a series like this one, they need to be
> considered standalone, I think.  The Linux kernel devs work differently
> than us here.

Zoltan broke them up because Michael asked him to do so.

...Robert

Re: CommitFest 2009-09, two weeks on

Michael Meskes <meskes@postgresql.org> — 2009-11-13T07:06:43Z

On Thu, Nov 12, 2009 at 03:07:27PM -0500, Robert Haas wrote:
> > If you want to submit patches in a series like this one, they need to be
> > considered standalone, I think.  The Linux kernel devs work differently
> > than us here.
> 
> Zoltan broke them up because Michael asked him to do so.

Actually these patchsets add different features. I see no reason why they
should be done as one patch. However, I haven't had the time to look into the
latest ones, but at least that was the situation when I asked Zoltan to split
the patch.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: CommitFest 2009-09, two weeks on

Hans-Juergen Schoenig <hs@cybertec.at> — 2009-11-13T08:52:37Z

On Nov 13, 2009, at 8:06 AM, Michael Meskes wrote:

> On Thu, Nov 12, 2009 at 03:07:27PM -0500, Robert Haas wrote:
>>> If you want to submit patches in a series like this one, they need  
>>> to be
>>> considered standalone, I think.  The Linux kernel devs work  
>>> differently
>>> than us here.
>>
>> Zoltan broke them up because Michael asked him to do so.
>
> Actually these patchsets add different features. I see no reason why  
> they
> should be done as one patch. However, I haven't had the time to look  
> into the
> latest ones, but at least that was the situation when I asked Zoltan  
> to split
> the patch.
>
> 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@jabber.org
> VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux,  
> PostgreSQL
>



good morning,

are there some pending technical issues with those patches or can we  
basically review and commit?

	many thanks,

		hans


--
Cybertec Schönig & Schönig GmbH
Reyergasse 9 / 2
A-2700 Wiener Neustadt
Web: www.postgresql-support.de

Re: CommitFest 2009-09, two weeks on

Robert Haas <robertmhaas@gmail.com> — 2009-11-13T12:14:37Z

2009/11/13 Hans-Jürgen Schönig <hs@cybertec.at>:
>
> On Nov 13, 2009, at 8:06 AM, Michael Meskes wrote:
>
>> On Thu, Nov 12, 2009 at 03:07:27PM -0500, Robert Haas wrote:
>>>>
>>>> If you want to submit patches in a series like this one, they need to be
>>>> considered standalone, I think.  The Linux kernel devs work differently
>>>> than us here.
>>>
>>> Zoltan broke them up because Michael asked him to do so.
>>
>> Actually these patchsets add different features. I see no reason why they
>> should be done as one patch. However, I haven't had the time to look into
>> the
>> latest ones, but at least that was the situation when I asked Zoltan to
>> split
>> the patch.
>>
>> 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@jabber.org
>> VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL
>>
>
>
>
> good morning,
>
> are there some pending technical issues with those patches or can we
> basically review and commit?

*scratches head*

How is anyone supposed to answer that question?  It is in the process
of reviewing them that one decides whether there are any technical
issues...

...Robert

Re: CommitFest 2009-09, two weeks on

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-13T13:35:10Z

Alvaro Herrera írta:
> Boszormenyi Zoltan escribió:
>   
>> Alvaro Herrera írta:
>>     
>>> I have applied this patch after some tinkering.  I mainly added support
>>> for "fetch_args: FORWARD opt_from_in name" and "BACKWARD opt_from_in
>>> name" in ecpg.addons which apparently you forgot.
>>>       
>> Thanks. Your fix is correct if this patch is considered
>> standalone. This means I have to re-post the later
>> patches to fix the reject this fix causes in them.
>>     
>
> Yeah.  BTW I don't think the rest of the pieces in this series make
> sense to apply separately, because they don't do anything useful by
> themselves (one of them introduces an unused function, what good is in
> that?).  I think they should be submitted as a single patch.
>
> If you want to submit patches in a series like this one, they need to be
> considered standalone, I think.  The Linux kernel devs work differently
> than us here.
>   

Dan started reviewing the dynamic cursorname patch.
He looked at it in the original form and he said that
he's not familiar with the ECPG code.

I have drafted the docs for the generated ECPG grammar
(it was applied mainstream by Michael shortly after
being posted) and have split this patch in question to help
Dan in the review. The patch pieces explain the various
problems about the implementation.

Is it really *that* apparent that I read too much LKML? :-D

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

ECPG patch 1, dynamic cursorname

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-16T10:58:04Z

Hi,

I have rebased the ECPG patches to current CVS.
This mail contains the dynamic cursorname,
the fine-grained split-up was re-united as per
Alvaro's comment.

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/

ECPG patch 2, SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-16T10:59:41Z

New version: rebased to current CVS.

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

Re: ECPG patch 3, DESCRIBE [OUTPUT] support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-16T11:00:44Z

New version: rebased to current CVS.

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

ECPG patch 4, out-of-scope cursor support in Informix-mode

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-16T11:05:22Z

New version: rebased to current CVS.

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

Re: ECPG patch 1, dynamic cursorname

Michael Meskes <meskes@postgresql.org> — 2009-11-26T15:07:54Z

On Mon, Nov 16, 2009 at 11:58:04AM +0100, Boszormenyi Zoltan wrote:
> I have rebased the ECPG patches to current CVS.
> This mail contains the dynamic cursorname,
> the fine-grained split-up was re-united as per
> Alvaro's comment.

Rest of it committed to HEAD.

Thanks for your work.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG patch 1, dynamic cursorname

Boszormenyi Zoltan <zb@cybertec.at> — 2009-11-26T18:27:04Z

Michael Meskes írta:
> On Mon, Nov 16, 2009 at 11:58:04AM +0100, Boszormenyi Zoltan wrote:
>   
>> I have rebased the ECPG patches to current CVS.
>> This mail contains the dynamic cursorname,
>> the fine-grained split-up was re-united as per
>> Alvaro's comment.
>>     
>
> Rest of it committed to HEAD.
>
> Thanks for your work.
>
> Michael
>   

Thanks very much for committing this.

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/

Re: ECPG patch 2, SQLDA support

Jaime Casanova <jcasanov@systemguards.com.ec> — 2009-12-08T21:12:44Z

On Mon, Nov 16, 2009 at 5:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> New version: rebased to current CVS.
>

This one no longer applies to HEAD, could you update it please?

-- 
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157

Re: ECPG patch 2, SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-14T09:35:33Z

Hi,

Jaime Casanova írta:
> On Mon, Nov 16, 2009 at 5:59 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> New version: rebased to current CVS.
>>
>>     
>
> This one no longer applies to HEAD, could you update it please?
>   

Will post a new version soon, Michael asked to turn it
into an ECPG native feature instead of compat-mode-only.

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/

Re: ECPG patch 2, SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-14T12:10:22Z

Boszormenyi Zoltan írta:
> Will post a new version soon, Michael asked to turn it
> into an ECPG native feature instead of compat-mode-only.
>   

Attached is the new version that applies cleanly to HEAD.

Changes:
- SQLDA is now an ECPG native feature, not compat-only.
- As a consequence, and to be coherent with the Informix syntax,
  keyword "SQL" in SQL DESCRIPTOR is now mandatory
  to denote the named SQL descriptor. "DESCRIPTOR"
  without "SQL" means SQLDA descriptor
- SQLDA can now return more than one tuples using
  the ->desc_next pointer
- Bcause SQLDA is now a native feature, there are two
  sqlda.pgc regression tests, to test compat-only syntax in
  compat mode and multi-tuple return values in native mode.
- sqlda->sqlvar[i].sqltype uses ECPGt_* type symbols,
  no more funny-looking typedefs in sqltypes.h, it's a clean
  compat-only header, as it was before

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/

Re: ECPG patch 3, DESCRIBE [OUTPUT] support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-14T13:11:40Z

New version, rebased to the previously sent
new SQLDA patch.

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

Re: ECPG patch 2, SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T09:30:59Z

Boszormenyi Zoltan írta:
> Boszormenyi Zoltan írta:
>   
>> Will post a new version soon, Michael asked to turn it
>> into an ECPG native feature instead of compat-mode-only.
>>   
>>     
>
> Attached is the new version that applies cleanly to HEAD.
>
> Changes:
> - SQLDA is now an ECPG native feature, not compat-only.
> - As a consequence, and to be coherent with the Informix syntax,
>   keyword "SQL" in SQL DESCRIPTOR is now mandatory
>   to denote the named SQL descriptor. "DESCRIPTOR"
>   without "SQL" means SQLDA descriptor
> - SQLDA can now return more than one tuples using
>   the ->desc_next pointer
> - Bcause SQLDA is now a native feature, there are two
>   sqlda.pgc regression tests, to test compat-only syntax in
>   compat mode and multi-tuple return values in native mode.
> - sqlda->sqlvar[i].sqltype uses ECPGt_* type symbols,
>   no more funny-looking typedefs in sqltypes.h, it's a clean
>   compat-only header, as it was before
>   

New version, fixed some loose ends introduced by
the native mode and multi-tuple SQLDA.

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/

Re: ECPG patch 3, DESCRIBE [OUTPUT] support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T09:33:13Z

Boszormenyi Zoltan írta:
> New version, rebased to the previously sent
> new SQLDA patch.
>   

New version, fixed some loose ends introduced by
the native mode and multi-tuple SQLDA. No more
build_sqlda() in ecpglib/descriptor.c, common code
is factored out for ECPGt_descriptor and ECPGt_sqlda
in ECPGdescribe().

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

Re: ECPG patch 4, out-of-scope cursor support in Informix-mode

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T12:58:06Z

New version: rebased to current CVS and
fix the rejects caused by the newer SQLDA
and DESCRIBE patches. Note that it's still
compat-mode only, I will send a modified patch
in the next email that is ECPG-wide, so it can be
decided if the feature is wanted in native mode or not.

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

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T13:04:33Z

Boszormenyi Zoltan írta:
> New version: rebased to current CVS and
> fix the rejects caused by the newer SQLDA
> and DESCRIBE patches. Note that it's still
> compat-mode only, I will send a modified patch
> in the next email that is ECPG-wide, so it can be
> decided if the feature is wanted in native mode or not.

Attached. Changes against the compat-mode only patch are:
- ECPG_informix_set_var() and ECPG_informix_get_var()
  were renamed to ECPGset_var() and ECPGget_var() and
  were moved into libecpg. The previous calls are kept
  in libecpg_compat.so as wrappers for compatibility reasons.
- adjust_informix() was renamed as adjust_outofscope_cursor_vars()
  to indicate its real purpose. The comment at the beginning
  of the function is modified to explain why it's useful.

The code produced by this is not too ugly this way in
native mode, only some ECPGset_var() calls are added
when the DECLARE are in the same function as OPEN/FETCH/CLOSE.
Global variables used in DECLARE aren't transformed at all.

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

Re: ECPG patch N+1, fix auto-prepare

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T13:19:19Z

Hi,

here's another patch that aims to fix auto-prepare.

The reason is, that in the project porting from Informix,
a small test case that used a cursor and two small SELECTs
issued for every record retrieved by the cursor showed that
for this case, the ESQL compiled binary finished about 60%
faster then the ECPG compiled counterpart running against
PostgreSQL. The cursor retrieved a little over 60'000 records.

We have modified the test code to prepare the two SELECTs
and now the new test code was faster then the ESQL/Informix
code, parsing and planning the two small SELECTs had such
an accumulated runtime effect.

Then we looked at ECPG and discovered that it already has
the auto-prepare feature, and tried it using "ecpg -r prepare".

However, it turned out that the auto-prepare feature is
over-zealous, it tries to prepare statements that are rejected
by the server, returning -400 (ECPG_PGSQL). One example is

  char *sqlstr = "SELECT ...";

  EXEC SQL PREPARE stmt1 FROM :sqlstr;
  EXEC SQL DECLARE cur1 CURSOR FOR stmt1;

The attached patch is an attempt to make the preprocessor
only pass ECPGst_prepnormal when it's definitely appropriate,
i.e. only for DELETE, INSERT, UPDATE and SELECT(-like)
statements in the grammar.

Comments?

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/

Re: ECPG patch N+1, fix auto-prepare

Michael Meskes <meskes@postgresql.org> — 2009-12-15T15:42:34Z

On Tue, Dec 15, 2009 at 02:19:19PM +0100, Boszormenyi Zoltan wrote:
> here's another patch that aims to fix auto-prepare.
> ...
> --- pgsql.6/src/interfaces/ecpg/preproc/output.c	2009-12-15 13:12:37.000000000 +0100
> *************** hashline_number(void)
> *** 106,112 ****
>   }
>   
>   void
> ! output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st)
>   {
>   
>   	fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat, force_indicator, connection ? connection : "NULL", questionmarks);
> --- 106,112 ----
>   }
>   
>   void
> ! output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st, int auto_prepare)
>   {
>   
>   	fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat, force_indicator, connection ? connection : "NULL", questionmarks);

Why do you add another argument to output_statement? You should easily be able
to use the existing ECPG_statement_type argument for this. How about changing
ECPGst_normal to ECPGst_normal and ECPGst_nonprep or something like this? Or
did I miss something?

Besides I don't think it's a good idea to create a local variable overriding a
global one with the same name. 


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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG patch N+1, fix auto-prepare

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-15T16:51:25Z

Michael Meskes írta:
> On Tue, Dec 15, 2009 at 02:19:19PM +0100, Boszormenyi Zoltan wrote:
>> here's another patch that aims to fix auto-prepare.
>> ...
>> --- pgsql.6/src/interfaces/ecpg/preproc/output.c	2009-12-15 13:12:37.000000000 +0100
>> *************** hashline_number(void)
>> *** 106,112 ****
>>   }
>>   
>>   void
>> ! output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st)
>>   {
>>   
>>   	fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat, force_indicator, connection ? connection : "NULL", questionmarks);
>> --- 106,112 ----
>>   }
>>   
>>   void
>> ! output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st, int auto_prepare)
>>   {
>>   
>>   	fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat, force_indicator, connection ? connection : "NULL", questionmarks);
>
> Why do you add another argument to output_statement? You should easily be able
> to use the existing ECPG_statement_type argument for this. How about changing
> ECPGst_normal to ECPGst_normal and ECPGst_nonprep or something like this? Or
> did I miss something?
>
> Besides I don't think it's a good idea to create a local variable overriding a
> global one with the same name. 

OK, here's another approach. output_statement()'s interface
is kept as the original, and not this function decides which
value it uses. I also introduced
    static char *ecpg_statement_type_name[]
for the textual names of the ECPGst_* symbols to keep the
preprocessed code readable, and minimize the impact on the
regression tests. So output_statement() always emits
ECPGst_* symbols in the preprocessed code instead of
ECPGst_normal/prepnormal and numeric value for the
other two cases. This way only 7 regression tests' source
has changed instead of 45... There are less
    1 -> ECPGst_execute and
    2 -> ECPGst_exec_immediate
changes than
    ECPGst_normal -> 0
changes would have been if I chose emitting the numeric value.

Is it acceptable?

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/

Re: ECPG patch N+1, fix auto-prepare

Michael Meskes <meskes@postgresql.org> — 2009-12-16T10:14:03Z

> OK, here's another approach. output_statement()'s interface
> is kept as the original, and not this function decides which

I still think this could be solved more easily.

> value it uses. I also introduced
>     static char *ecpg_statement_type_name[]
> for the textual names of the ECPGst_* symbols to keep the
> preprocessed code readable, and minimize the impact on the
> regression tests. So output_statement() always emits
> ECPGst_* symbols in the preprocessed code instead of
> ECPGst_normal/prepnormal and numeric value for the
> other two cases. This way only 7 regression tests' source
> has changed instead of 45... There are less
>     1 -> ECPGst_execute and
>     2 -> ECPGst_exec_immediate
> changes than
>     ECPGst_normal -> 0
> changes would have been if I chose emitting the numeric value.
> 
> Is it acceptable?

Yes sure.

I changed some small parts of your patch (see above) and will commit in a few
minutes. Just running regression tests.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG patch N+1, fix auto-prepare

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-16T10:54:41Z

Michael Meskes írta:
>> OK, here's another approach. output_statement()'s interface
>> is kept as the original, and not this function decides which
>>     
>
> I still think this could be solved more easily.
>   

Thanks very much for committing it.

But I don't understand your change. My code was:

=====================================
ecpg.addons:
ECPG: stmtDeleteStmt block
ECPG: stmtInsertStmt block
ECPG: stmtSelectStmt block
ECPG: stmtUpdateStmt block
        { output_statement($1, 1, auto_prepare ? ECPGst_prepnormal :
ECPGst_normal); }
=====================================
output.c:
static char *ecpg_statement_type_name[] = {
        "ECPGst_normal",
        "ECPGst_execute",
        "ECPGst_exec_immediate",
        "ECPGst_prepnormal"
};

void
output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st)
{

        fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat,
force_indicator, connection ? connection : "NULL", questionmarks);
        if (st == ECPGst_normal || st == ECPGst_prepnormal)
        {
                fprintf(yyout, "%s, \"", ecpg_statement_type_name[st]);
                output_escaped_str(stmt, false);
                fputs("\", ", yyout);
        }
        else
                fprintf(yyout, "%s, %s, ", ecpg_statement_type_name[st],
stmt);
=====================================

So the ECPGst_normal vs. prepnormal is decided at the caller
and output_statement() is simplified a bit vs the original.

Your code is:

=====================================
ecpg.addons:
ECPG: stmtDeleteStmt block
ECPG: stmtInsertStmt block
ECPG: stmtSelectStmt block
ECPG: stmtUpdateStmt block
        { output_statement($1, 1, ECPGst_prepnormal); }
=====================================
output.c:
static char *ecpg_statement_type_name[] = {
        "ECPGst_normal",
        "ECPGst_execute",
        "ECPGst_exec_immediate",
        "ECPGst_prepnormal"
};

void
output_statement(char *stmt, int whenever_mode, enum ECPG_statement_type st)
{
        fprintf(yyout, "{ ECPGdo(__LINE__, %d, %d, %s, %d, ", compat,
force_indicator, connection ? connection : "NULL", questionmarks);
        if (st == ECPGst_execute || st == ECPGst_exec_immediate)
        {
                fprintf(yyout, "%s, %s, ", ecpg_statement_type_name[st],
stmt);
        }
        else
        {
                if (st == ECPGst_prepnormal && auto_prepare)
                        fputs("ECPGst_prepnormal, \"", yyout);
                else
                        fputs("ECPGst_normal, \"", yyout);

                output_escaped_str(stmt, false);
                fputs("\", ", yyout);
        }
=====================================

Your code in ecpg.addons calls output_statement()
unconditionally with ECPGst_prepnormal and
output_statement() decides what to do with the
"auto_prepare" global variable. Your code doesn't
seem more readable than mine, but does the same
with the currently existing callers.

>> value it uses. I also introduced
>>     static char *ecpg_statement_type_name[]
>> for the textual names of the ECPGst_* symbols to keep the
>> preprocessed code readable, and minimize the impact on the
>> regression tests. So output_statement() always emits
>> ECPGst_* symbols in the preprocessed code instead of
>> ECPGst_normal/prepnormal and numeric value for the
>> other two cases. This way only 7 regression tests' source
>> has changed instead of 45... There are less
>>     1 -> ECPGst_execute and
>>     2 -> ECPGst_exec_immediate
>> changes than
>>     ECPGst_normal -> 0
>> changes would have been if I chose emitting the numeric value.
>>
>> Is it acceptable?
>>     
>
> Yes sure.
>
> I changed some small parts of your patch (see above) and will commit in a few
> minutes. Just running regression tests.
>   

Okay, I have to rebase my SQLDA and DESCRIBE
patches again, since the regression tests' results may
have changed beause of this patch. I will post them soon.

Again, thanks for committing it.

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/

Re: ECPG patch N+1, fix auto-prepare

Michael Meskes <meskes@postgresql.org> — 2009-12-16T12:33:01Z

On Wed, Dec 16, 2009 at 11:54:41AM +0100, Boszormenyi Zoltan wrote:
> Your code in ecpg.addons calls output_statement()
> unconditionally with ECPGst_prepnormal and
> output_statement() decides what to do with the
> "auto_prepare" global variable. Your code doesn't
> seem more readable than mine, but does the same
> with the currently existing callers.

It better should do the same. :-)

Maybe finding it simpler this way just comes from me being used to the way this
part of the source code works. It's essantially the same, I only moved that one
auto_prepare test out of the parser.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG patch 2, SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-16T14:41:03Z

Rebased to current CVS HEAD after my auto-prepare fix
was added. The new regression tests' C source has changed
because of this.

The other two patches don't need re-posting, as
they apply with minimal fuzz and no rejects and
their regression tests work as-is.

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/

ECPG SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-04T18:39:14Z

Hi,

new patch attached.

Per Michael's request, a DB2 / Sybase compatible
SQLDA structure is implemented for ECPG's native mode.
The Informix-compatible structure is only for compat mode.
The sqlda.h header switches between the two different
structures depending on a new #define introduced in
and added to the generated C source by the preprocessor
in compat-mode. Also, per discussion with Michael,
     FETCH ... USING DESCRIPTOR sqlda
is also accepted by the grammar in native mode, too.

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/

ECPG DESCRIBE [OUTPUT] support

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-04T18:51:51Z

Hi,

new patch is attached. Modified according to
the new DB2 / Sybase compatible SQLDA structure.
ECPGdescribe() has an "int compat" parameter, because:
- the (struct prepared_statement *)->stmt ->compat is not set, and
- it's more sensible to use the compat mode of the
  ECPGdescribe() caller, because different source files can be
  compiled with different (native or compat) modes.
The Informix-specific syntax "DESCRIBE ... INTO sqlda" is also
accepted in native mode now.

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/

ECPG out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-04T18:54:33Z

Hi,

new patch attached, rebased to the latest
SQLDA and DESCRIBE patches.
This is the variant with support in native mode.

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/

Re: ECPG SQLDA support

Alvaro Herrera <alvherre@commandprompt.com> — 2010-01-04T18:59:59Z

Boszormenyi Zoltan wrote:

I happened to notice this by chance:

> + #if (LONG_BIT == 64)
> + #define	SQLINT8		ECPGt_long
> + #define	SQLSERIAL8	ECPGt_long
> + #else
> + #define	SQLINT8		ECPGt_long_long
> + #define	SQLSERIAL8	ECPGt_long_long
> + #endif

I'm not sure how portable is the LONG_BIT business.  We don't seem to
use it anywhere else (hmm, but then we do use CHAR_BIT elsewhere)

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: ECPG SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-04T19:13:52Z

Alvaro Herrera írta:
> Boszormenyi Zoltan wrote:
>
> I happened to notice this by chance:
>
>   
>> + #if (LONG_BIT == 64)
>> + #define	SQLINT8		ECPGt_long
>> + #define	SQLSERIAL8	ECPGt_long
>> + #else
>> + #define	SQLINT8		ECPGt_long_long
>> + #define	SQLSERIAL8	ECPGt_long_long
>> + #endif
>>     
>
> I'm not sure how portable is the LONG_BIT business.  We don't seem to
> use it anywhere else (hmm, but then we do use CHAR_BIT elsewhere)
>   

I specifically looked for a portable solution, as
    #if sizeof(...) == N
is not evaluated at compile time.

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/

Re: ECPG SQLDA support

Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-04T19:32:56Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> I'm not sure how portable is the LONG_BIT business.

I think checking SIZEOF_LONG would be preferred, since that's what
we use elsewhere.  Although actually I wonder why this code exists
at all --- wouldn't it be easier to make these depend on "int64"?

			regards, tom lane

Re: ECPG SQLDA support

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-04T19:42:51Z

Tom Lane írta:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>   
>> I'm not sure how portable is the LONG_BIT business.
>>     
>
> I think checking SIZEOF_LONG would be preferred, since that's what
> we use elsewhere.  Although actually I wonder why this code exists
> at all --- wouldn't it be easier to make these depend on "int64"?
>
> 			regards, tom lane
>   

Don't ask me why ECPGt_long_long and ECPGt_unsigned_long_long
exist. But they do, and the libecpg code has some
    #ifdef HAVE_LONG_LONG_INT_64
surrounding code handling them. Maybe it would've been better to be
consistent with that coding.

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

Re: ECPG DESCRIBE [OUTPUT] support

Robert Haas <robertmhaas@gmail.com> — 2010-01-05T01:35:50Z

On Mon, Jan 4, 2010 at 1:51 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> new patch is attached. Modified according to
> the new DB2 / Sybase compatible SQLDA structure.
> ECPGdescribe() has an "int compat" parameter, because:
> - the (struct prepared_statement *)->stmt ->compat is not set, and
> - it's more sensible to use the compat mode of the
>  ECPGdescribe() caller, because different source files can be
>  compiled with different (native or compat) modes.
> The Informix-specific syntax "DESCRIBE ... INTO sqlda" is also
> accepted in native mode now.

For tracking purposes, can you add a "Patch"-type comment for each of
these to the relevant entry on commitfest.postgresql.org?

Thanks,

...Robert

Re: ECPG SQLDA support

Michael Meskes <meskes@postgresql.org> — 2010-01-05T16:53:15Z

On Mon, Jan 04, 2010 at 07:39:14PM +0100, Boszormenyi Zoltan wrote:
> new patch attached.
> ...

Great job Zoltan, committed.

> The sqlda.h header switches between the two different
> structures depending on a new #define introduced in
> and added to the generated C source by the preprocessor

I changed this to use the already existing _ECPG_INFORMIX_H define.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG SQLDA support

Michael Meskes <meskes@postgresql.org> — 2010-01-05T16:54:12Z

On Mon, Jan 04, 2010 at 02:32:56PM -0500, Tom Lane wrote:
> I think checking SIZEOF_LONG would be preferred, since that's what
> we use elsewhere.  Although actually I wonder why this code exists
> at all --- wouldn't it be easier to make these depend on "int64"?

It does use int64. However, ecpg uses HAVE_LONG_LONG_INT64 to decide whether
it's datatype ECPGt_long_long exists. I changed the patch to use the same
define as usual.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

Re: ECPG DESCRIBE [OUTPUT] support

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-15T12:16:18Z

Hi,

I just saw that you committed the DESCRIBE patch.

Please, also add this small change that adds ecpg_raise()
calls to ECPGdescribe() to return the proper sqlca error
in error paths for:
- unsupported call for DESCRIBE INPUT
- no such connection name
- no such prepared statement

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/

Re: ECPG DESCRIBE [OUTPUT] support

Michael Meskes <meskes@postgresql.org> — 2010-01-15T13:19:30Z

On Fri, Jan 15, 2010 at 01:16:18PM +0100, Boszormenyi Zoltan wrote:
> Please, also add this small change that adds ecpg_raise()
> calls to ECPGdescribe() to return the proper sqlca error
> in error paths for:
> ...

Done.

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@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL

ECPG documentation patch

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-15T18:08:05Z

Hi,

here's the documentation patch for the new ECPG features.

- I changed the order of sections "Using Descriptor Areas" and
  "Informix compatibility mode"
- split the "Using Descriptor Areas", so it now have two subsections:
  "Named SQL Descriptor Areas" and "SQLDA Descriptor Areas".
  The second one talks about the native mode SQLDA only.
- Documented DESCRIBE and the USING/INTO quirks.
- Documented the "string" pseudo-type in compat mode
- Modified the section name "Additional embedded SQL statements",
  it now reads "Additional/missing embedded SQL statements" and
  documented the lack of "FREE cursor_name" statement and
  the behaviour of "FREE statement_name" statement.
- Documented the Informix-compatible SQLDA under the
  "Informix compatibility mode" section.

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/

Fix auto-prepare #2

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-18T10:58:38Z

Hi,

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. The application gets back a error code -201 which seems bogus
and it turned out to be a missing
    return (false);
after ecpg_raise(ECPG_SQLSTATE_INVALID_SQL_STATEMENT_NAME)
on line 1756 in execute.c, so the error handling later in the same function
masqueraded this error. But fixing this doesn't solve the main problem
that the query is in the auto-prepared query cache but not is not prepared
on the server if the connection is closed and re-opened.

The same problem may also arise if the same query is used over multiple
simultaneous connections.

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.

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/

Re: Fix auto-prepare #2

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2010-01-19T05:23:02Z

Hi, I'm reviewing your patch and have a couple of comments.

Boszormenyi Zoltan <zb@cybertec.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?

> 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(). Prepared stetements also might
leak in a error path. I tryd to fix both of them in the attached
"6-pg85-fix-auto-prepare-multiconn-3.patch", but please recheck the issues.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Re: Fix auto-prepare #2

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-19T11:01:24Z

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@cybertec.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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-19T16:58:16Z

Zoltan,

while testing your patch I went through the test cases and found this in outofscope.pgc:

> + #include <inttypes.h>

As we know by now this won't work. :-)

Besides, would you mind simplifying the test case a little bit? There is no
need to have it test all the sqlda stuff, too. I don't mind having two cases
testing sqlda but this makes testing out of scope variable handling more
difficult especially with sqlda being a rather complex system with different
struct definitions and so on.

Also I wonder why you added struct.pgc. It seems to build and run without a
problem on a non-patched system. Is it an additional test that just happened to
be included here?

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-19T17:45:58Z

Michael Meskes írta:
> Zoltan,
>
> while testing your patch I went through the test cases and found this in outofscope.pgc:
>
>   
>> + #include <inttypes.h>
>>     
>
> As we know by now this won't work. :-)
>   

Okay, I will fix it. :-) I forgot it's in there as well.

> Besides, would you mind simplifying the test case a little bit? There is no
> need to have it test all the sqlda stuff, too. I don't mind having two cases
> testing sqlda but this makes testing out of scope variable handling more
> difficult especially with sqlda being a rather complex system with different
> struct definitions and so on.
>   

Okay.

> Also I wonder why you added struct.pgc. It seems to build and run without a
> problem on a non-patched system. Is it an additional test that just happened to
> be included here?
>   

It was included in there because the first symptom
that lead to this patch under Informix compat mode that
a simple struct variable wasn't properly unrolled because
adjust_informix() lost informiation about the type.
Did I accidentally move it to native mode? I'll move it back.

I will send an updated 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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-19T18:40:54Z

Boszormenyi Zoltan írta:
> I will send an updated patch.
>   

Attached with the requested modifications.

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/

Re: Fix auto-prepare #2

Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2010-01-21T02:02:16Z

Boszormenyi Zoltan <zb@cybertec.at> wrote:

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

Good. I think the patch is ready to commit.

A comment for committer (Michael?) :
I was cofused by the AddStmtToCache's 2nd argument "char *stmtID"
because it doesn't have a const. Should it be "const char *" ?
If the argument has a const, callers assume that they can pass
a not-strdup'ed string as the argument.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Re: Fix auto-prepare #2

Michael Meskes <meskes@postgresql.org> — 2010-01-22T14:18:44Z

Takahiro-san,

> Good. I think the patch is ready to commit.

Thanks for reviewing it. I just committed the patch.

> A comment for committer (Michael?) :
> I was cofused by the AddStmtToCache's 2nd argument "char *stmtID"
> because it doesn't have a const. Should it be "const char *" ?
> If the argument has a const, callers assume that they can pass
> a not-strdup'ed string as the argument.

Valid point, I can see no reason for not making this a "const char *". So let's
try.

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-22T14:52:37Z

> diff -dcrpN pgsql.orig/src/interfaces/ecpg/test/expected/compat_informix-struct.c pgsql.4.1/src/interfaces/ecpg/test/expected/compat_informix-struct.c
> ...
> + 	/* Test DECLARE ... SELECT ... INTO with struct type */
> + 
> + 	ECPGset_var( 0, &( myvar ), __LINE__);\
> +  ECPGset_var( 1, &( mynullvar ), __LINE__);\
> +  ECPG_informix_reset_sqlca(); /* declare mycur cursor for select * from a1 */
> + #line 45 "struct.pgc"
> + 
> + 	{ ECPGdo(__LINE__, 1, 1, NULL, 0, ECPGst_normal, "declare mycur cursor for select * from a1", ECPGt_EOIT, 
> + 	ECPGt_int,&(myvar.id),(long)1,(long)1,sizeof(int), 
> ...

Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? 

Michael

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-22T17:11:51Z

Michael Meskes írta:
>> diff -dcrpN pgsql.orig/src/interfaces/ecpg/test/expected/compat_informix-struct.c pgsql.4.1/src/interfaces/ecpg/test/expected/compat_informix-struct.c
>> ...
>> + 	/* Test DECLARE ... SELECT ... INTO with struct type */
>> + 
>> + 	ECPGset_var( 0, &( myvar ), __LINE__);\
>> +  ECPGset_var( 1, &( mynullvar ), __LINE__);\
>> +  ECPG_informix_reset_sqlca(); /* declare mycur cursor for select * from a1 */
>> + #line 45 "struct.pgc"
>> + 
>> + 	{ ECPGdo(__LINE__, 1, 1, NULL, 0, ECPGst_normal, "declare mycur cursor for select * from a1", ECPGt_EOIT, 
>> + 	ECPGt_int,&(myvar.id),(long)1,(long)1,sizeof(int), 
>> ...
>>     
>
> Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? 
>   

Because there's no ECPGget_var()s emitted for
- global variables
- variables in the same function

ECPGget_var() is only used in case the cursor declaration
used INTO/USING and it's in a different function from
the one where OPEN/FETCH/CLOSE reside. But this
cannot be determined easily, e.g. short of making ECPG
a two-pass precompiler, so ECPGset_var() is always used.

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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-24T14:23:24Z

On Fri, Jan 22, 2010 at 06:11:51PM +0100, Boszormenyi Zoltan wrote:
> > Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? 
> >   
> 
> Because there's no ECPGget_var()s emitted for
> - global variables
> - variables in the same function
> 
> ECPGget_var() is only used in case the cursor declaration
> used INTO/USING and it's in a different function from
> the one where OPEN/FETCH/CLOSE reside. But this
> cannot be determined easily, e.g. short of making ECPG
> a two-pass precompiler, so ECPGset_var() is always used.

This shows some well thought implementation. But what I was wondering about was
why this is used as test case. While I see the need to test this part of ecpg I
thought this test case was added because it showed a problem without your
changes.

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-24T18:25:24Z

Michael Meskes írta:
> On Fri, Jan 22, 2010 at 06:11:51PM +0100, Boszormenyi Zoltan wrote:
>   
>>> Why does the preproc spit out ECPGset_var's but no ECPGget_var's in this test case? 
>>>   
>>>       
>> Because there's no ECPGget_var()s emitted for
>> - global variables
>> - variables in the same function
>>
>> ECPGget_var() is only used in case the cursor declaration
>> used INTO/USING and it's in a different function from
>> the one where OPEN/FETCH/CLOSE reside. But this
>> cannot be determined easily, e.g. short of making ECPG
>> a two-pass precompiler, so ECPGset_var() is always used.
>>     
>
> This shows some well thought implementation.

Thanks. But the truth is that your comment about "this
adjust_informix() hack goes into native mode only over
my dead body" made me come up with this change and
the reasoning. :-) Thus if some code with cursors does
everything in one function then only some useless
ECPGset_var() calls are added by the preprocessor,
making the change the least intrusive for old regression tests.

>  But what I was wondering about was
> why this is used as test case. While I see the need to test this part of ecpg I
> thought this test case was added because it showed a problem without your
> changes.
>   

The problem that popped up first was that adjust_informix()
didn't properly deal with structs, remember? I tried some
small changes to fix that but they turned out to be improper
ones. The small changes worked for the initial problem,
i.e. IIRC in some cases adjust_informix() was bypassed
and the struct/union members were unrolled properly
for Informix compat mode, too. The regression test
"compat_informix/struct.pgc" shows this case. Then our
client showed a much larger programme which actually
used cursors in an event driver way. E.g. PgUp/PgDn
called different functions that contained only FETCH NEXT
or FETCH PRIOR. This was a curses based terminal programme
and my first little patch that solved only the struct unrolling case
failed in this case, because either ECPG segfaulted in
adjust_informix() or the code that ECPG produced for the
FETCH statements didn't have the proper output variables.
This is lead to the changes in the patch, which can be
summarized as:
1. track function names via the lexer (to minimize the impact
   of ECPGget_var(), now we can compare the function name
   where the cursor was DECLAREd and is used)
2. track type names for structs/unions to be able to emit
    (* ( typename *) &var) in adjust_informix(), and
3. rewriting adjust_informix() to handle struct/union

The result is that cursors handling statements (OPEN/FETCH/MOVE)
can now be safely put into a different function from where
it was DECLAREd. And this makes possible to use ECPG
in event driven code, which is a feature that I think worth
considering making available in native mode, too. This is
why adjust_informix() was also renamed. And while I agree
with your comment about that "this can lead to dangerous
programming", I think the only thing needed is to document
the safety borders, not to prevent crossing them. C is about
"living la vida loca". ;-)

The regression test introduced (preproc/outofscope.pgc) tries
to show this functionality. Currently (without the patch) this
test makes ECPG abort in ecpg_type_name().

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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-25T18:19:24Z

On Sun, Jan 24, 2010 at 07:25:24PM +0100, Boszormenyi Zoltan wrote:
> The problem that popped up first was that adjust_informix()
> didn't properly deal with structs, remember? I tried some

Yes, that's what made me wondering.

> i.e. IIRC in some cases adjust_informix() was bypassed
> and the struct/union members were unrolled properly
> for Informix compat mode, too. The regression test
> "compat_informix/struct.pgc" shows this case. Then our

Now this is what I don't get. Shouldn't this test use different functions to
really show this problem? Isn't it hidden now by the new functionality of not
spitting out ECPGget_var's?

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-25T18:52:05Z

Michael Meskes írta:
> On Sun, Jan 24, 2010 at 07:25:24PM +0100, Boszormenyi Zoltan wrote:
>   
>> The problem that popped up first was that adjust_informix()
>> didn't properly deal with structs, remember? I tried some
>>     
>
> Yes, that's what made me wondering.
>
>   
>> i.e. IIRC in some cases adjust_informix() was bypassed
>> and the struct/union members were unrolled properly
>> for Informix compat mode, too. The regression test
>> "compat_informix/struct.pgc" shows this case. Then our
>>     
>
> Now this is what I don't get.

I may have been unclear. My first attempts at solving it
either basically bypassed adjust_informix() or tried to
unroll the structs *before* calling adjust_informix().
IIRC, your comment about that solution was something
like "unrolling should be done at only one place centrally".
Which I agreed after learning what dump_variables() and
ECPGdump_a_type() do.

>  Shouldn't this test use different functions to
> really show this problem?

I don't think so. The problem only happened in case of
cursors because this was the only case when adjust_informix()
was called and the lack of handling struct/union there
was the problem. The final "else" branch used ecpg_type_name()
which calls abort() in case of unknown types.

>  Isn't it hidden now by the new functionality of not
> spitting out ECPGget_var's?
>   

No. This problem is hidden by the fact the adjust_informix()
(now adjust_outofscope_cursor_vars()) now handles structs/unions
properly and the struct members are properly unrolled by
ECPGdump_a_type().

The "not spitting out ECPGget_var()" part is about tidying
the preprocessed C source. As I wrote previously, globals
don't need transformation and nor the locals in case the
OPEN/FETCH statements are in the same function as DECLARE.

But considering all the above, we might not need the new
compat_informix/struct.pgc regression test. I think it was tested
already in e.g. preproc/array_of_struct.pgc and preproc/type.pgc
and the new feature (if accepted) is a unified one, it would show
the problem for native mode 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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-25T19:01:50Z

On Mon, Jan 25, 2010 at 07:52:05PM +0100, Boszormenyi Zoltan wrote:
> But considering all the above, we might not need the new
> compat_informix/struct.pgc regression test. I think it was tested
> already in e.g. preproc/array_of_struct.pgc and preproc/type.pgc
> and the new feature (if accepted) is a unified one, it would show
> the problem for native mode as well.

That was my feeling too and the reason for these questions. Again, this has
nothing to do with the feature you implemented, it was just about this special
test.

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-25T19:17:26Z

Michael Meskes írta:
> On Mon, Jan 25, 2010 at 07:52:05PM +0100, Boszormenyi Zoltan wrote:
>   
>> But considering all the above, we might not need the new
>> compat_informix/struct.pgc regression test. I think it was tested
>> already in e.g. preproc/array_of_struct.pgc and preproc/type.pgc
>> and the new feature (if accepted) is a unified one, it would show
>> the problem for native mode as well.
>>     
>
> That was my feeling too and the reason for these questions. Again, this has
> nothing to do with the feature you implemented, it was just about this special
> test.
>   

Should I send you a new patch without this regression test
or do you delete it before applying the patch?

BTW, thank you very much for the review.

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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-26T08:46:10Z

> Should I send you a new patch without this regression test
> or do you delete it before applying the patch?

Na, I will just remove it, no need to worry.

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-26T09:56:52Z

Michael Meskes írta:
>> Should I send you a new patch without this regression test
>> or do you delete it before applying the patch?
>>     
>
> Na, I will just remove it, no need to worry.
>
> Michael
>   

Thanks for applying it. You seem to have accidentally
removed the outofscope.pgc test, too. The test results are there
and the test is wired up in Makefile and in ecpg_schedule[_tcp]
but the actual regression test code is missing:

make[2]: *** No rule to make target `outofscope', needed by `all'.  Stop.
make[2]: Leaving directory
`/home/zozo/Schönig-számlák/leoni/2/new/new/new/8/pgsql/src/interfaces/ecpg/test/preproc'
make[1]: *** [all] Error 2
make[1]: Leaving directory
`/home/zozo/Schönig-számlák/leoni/2/new/new/new/8/pgsql/src/interfaces/ecpg/test'
make: *** [check] Error 2

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/

Re: ECPG patch 4.1, out-of-scope cursor support in native mode

Michael Meskes <meskes@postgresql.org> — 2010-01-26T10:29:44Z

On Tue, Jan 26, 2010 at 10:56:52AM +0100, Boszormenyi Zoltan wrote:
> Thanks for applying it. You seem to have accidentally
> removed the outofscope.pgc test, too. The test results are there

Yup, my bad. I'm already trying to recover and testing. Apparently the files
weren't added but I didn't notice. 

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@jabber.org
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL