Re: pgAgent: C++ Port - Patch Review - Mailing list pgadmin-hackers

From Linreg
Subject Re: pgAgent: C++ Port - Patch Review
Date
Msg-id 2647492.OYVUvMgy6a@wolfclan.ang.de
Whole thread Raw
In response to Re: pgAgent: C++ Port - Patch Review  (Neel Patel <neel.patel@enterprisedb.com>)
List pgadmin-hackers

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.patel@enterprisedb.com>wrote:

> > Hi,

> >

> > On Thu, Oct 24, 2013 at 2:59 PM, Linreg <linreg@gmx.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

 

pgadmin-hackers by date:

Previous
From: Linreg
Date:
Subject: Re: pgAgent: C++ Port - Patch Review
Next
From: Akshay Joshi
Date:
Subject: Re: PATCH: Improvement in the debugger support for EnterpriseDB <= 9.2