Re: pgAgent: C++ Port - Patch Review

Hi,

windows port:
okay, I'll try to make it until next week

Am Freitag, 25. Oktober 2013, 12:51:16 schrieb Neel Patel:
> Hi,
>
> One more thing we forgot to add.
>
> Windows part is not working.
>
> @Thomas: Are you going to fix windows part along with this review comments
> and send the updated patch ?

Yes. Okay.

> Thanks,
> Neel Patel
>
> On Fri, Oct 25, 2013 at 12:04 PM, Neel Patel <neel(dot)patel(at)enterprisedb(dot)com>wrote:
> > Hi,
> >
> > On Thu, Oct 24, 2013 at 2:59 PM, Linreg <linreg(at)gmx(dot)net> wrote:
> >> **
> >>
> >> Am Donnerstag, 24. Oktober 2013, 11:11:21 schrieb Neel Patel:
> >> > Hi,
> >> >
> >> >
> >> >
> >> > Below are the review comments. Compile and tested in Linux.
> >> >
> >> >
> >> >
> >> > 1) During compilation we got the following error in "connection.h"
> >> >
> >> >
> >> >
> >> > error: pgsql/libpq-fe.h: No such file or directory
> >> >
> >> >
> >> >
> >> > To solve the above error we changed "#include <libpg-fe.h>" instead of
> >> >
> >> > "#include <pgsql/libpq-fe.h>" and it is working fine. Correct me if it
> >>
> >> is
> >>
> >> > some configuration issue.
> >> >
> >> >
> >> >
> >> > 2) We are getting the below warning in unix.cpp file which needs to
> >> > fix.
> >> >
> >> >
> >> >
> >> > warning: the use of `tmpnam' is dangerous, better use `mkstemp'.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > 3) Some of the code is commented and not used so remove the unnecessary
> >> >
> >> > code.
> >> >
> >> >
> >> >
> >> > 4) README file should be modified according to new changes.
> >> >
> >> >
> >> >
> >> > 5) In Execute() method in Job.cpp file, we should have to delete
> >> > "steps"
> >> >
> >> > from wherever we are returning otherwise it will create the memory
> >> > leak.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > Thanks,
> >> >
> >> > Neel Patel
> >>
> >> 1.)
> >>
> >> it is system dependent. On my SUSE system pibpg-header to be
> >> installed under the pgsql directory.
> >>
> >> Example: /usr/include/pgsql/libpq-fe.h
> >
> > As we discussed we should remove the pgsql/ prefix.
> >
> >> 2.)
> >>
> >> I changed tmpnam to mkdtemp.
> >>
> >> I hope thats is okay
> >
> > OK.
> >
> >> 3.)
> >>
> >> Can you tell me the name of the files. I will delete comments.
> >
> > connection.cpp
> > job.cpp
> > misc.cpp
> > pgAgent.h
> >
> > Also check other files for commented codes.
> >
> >> 4.)
> >>
> >> its also my job?
> >
> > I think so because you have only implemented this feature so you are the
> > best person to do it. :)
> >
> >> 5.)
> >>
> >> I have corrected. In the default branch of the switch statement was
> >> a return statement(Job.cpp Line: 325). Before that i have
> >> a "delete steps," added
> >
> > That is fine. But what about return statement in Job.cpp:346 ?
> >
> >> Thomas Steffen