Re: pgAgent: C++ Port - Patch Review

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 ?

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