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 <[email protected]>wrote: > Hi, > > > On Thu, Oct 24, 2013 at 2:59 PM, Linreg <[email protected]> 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 >> >> >> > >
