Re: pgAgent: C++ Port - Patch Review - Mailing list pgadmin-hackers
From | Dave Page |
---|---|
Subject | Re: pgAgent: C++ Port - Patch Review |
Date | |
Msg-id | CA+OCxowR6uB9kM_t13u_6TD4=VoX-f_hP+SbE1EKQOhahXZxTw@mail.gmail.com Whole thread Raw |
In response to | Re: pgAgent: C++ Port - Patch Review (Linreg <linreg@gmx.net>) |
Responses |
Re: pgAgent: C++ Port - Patch Review
Re: pgAgent: C++ Port - Patch Review |
List | pgadmin-hackers |
Hi On Fri, Sep 13, 2013 at 11:43 AM, Linreg <linreg@gmx.net> wrote: > Hi Dave, > > > > - The new code doesn't match the existing coding style - you've used 4 > > spaces instead of tabs for indents, and braces to open a new context > > are not on a new line for example. Please follow the existing coding > > style to make the diffs (much) easier to read. You might also try > > something like: astyle -n -p -b -S -t4 -k3 -z2 `find . -name "*.cpp" > > -o -name "*.h"` > > > > => done with astyle OK. > - There are numerous comments that are clearly notes to yourself, and > > lines of code that have been commented out. This needs to be cleaned > > up before anything can be committed. > > > > => cleaned up OK. > - There are no updates to the build system, which seems essential for > > this patch. As a result, I can't compile the code at all yet. > > > > => done. wx settings are out and sdtc++ are in. > > => cmake and make running without warnings or errors > > => My Plattform: > > 64-Bit Linux 3.7.10-1.16 > > openSuse Distro > > gcc 4.7.2 with std=c++11 That's not playing nicely with my compiler: viper:pgagent dpage$ make all Scanning dependencies of target pgagent [ 14%] [ 28%] [ 42%] [ 57%] Building CXX object CMakeFiles/pgagent.dir/job.cpp.o Building CXX object CMakeFiles/pgagent.dir/connection.cpp.o Building CXX object CMakeFiles/pgagent.dir/misc.cpp.o Building CXX object CMakeFiles/pgagent.dir/pgAgent.cpp.o cc1plus: error: unrecognized command line option "-std=c++11"cc1plus: error: unrecognized command line option "-std=c++11" cc1plus: error: unrecognized command line option "-std=c++11" cc1plus: error: unrecognized command line option "-std=c++11" make[2]: *** [CMakeFiles/pgagent.dir/connection.cpp.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: *** [CMakeFiles/pgagent.dir/misc.cpp.o] Error 1 make[2]: *** [CMakeFiles/pgagent.dir/pgAgent.cpp.o] Error 1 make[2]: *** [CMakeFiles/pgagent.dir/job.cpp.o] Error 1 make[1]: *** [CMakeFiles/pgagent.dir/all] Error 2 make: *** [all] Error 2 That's with i686-apple-darwin11-llvm-g++-4.2 (GCC) 4.2.1 (Based on Apple Inc. build 5658) (LLVM build 2336.11.00), the default compiler on Mac OS X Mountain Lion. FYI, we need to support much older compilers than that, for example, g++ (GCC) 4.1.2 20080704 (Red Hat 4.1.2-54) ships with RHEL 5, which is a supported platform. > - You seem to have hard-coded the exit code from Windows batch job > > steps to 1. Why? > > > > => i changed back to git-head version. i believe it come from old 3.3.0 > source code version. I will never make such things. I'm working on git-head (d6c5a807110a7ec6ecde9ad59dd7e3b35891fb5e), and see this when I apply your patch: - GetExitCodeProcess(h_process, (LPDWORD)&rc); - CloseHandle(h_process); CloseHandle(h_script); - + rc = 1; This is clearly changing the logic here, which it should not. > - You also seem to have removed the connection pool. Why? > > > > Why not? > > Pos: > > + The code is simpler (no locking at all, fewer code) > > + easier maintenance > > > > Neg: > > - i'm not sure. maybe more parallel connection. > > > > There are two scenarios: > > normal typ: two or three dozen job. only a few running in parallel > > > > big typ: many many jobs. many running in parallel. This companies have other > problems or use a connection pooler like pgpool. > > > > In summary the positive points weigh more heavily This change is unrelated to porting to pure C++, and needs to be discussed and (if acceptable) implemented as a separate patch. I'm not convinced it's an appropriate change at all - I certainly work with customer who do not use a connection pooler for various reasons, and rely on the pooler in the agent to prevent large numbers of connect/disconnect cycles, which amongst other things use resources unnecessarily, and can fill up audit logs. Another problem that I've noticed is that you've unconditionally changed the logging format to be pipe delimited. This is also not acceptable as part of this patch, and should be discussed and implemented separately. At minimum, this would need to be a configurable behaviour change, and by default, I would want it to implement standard (comma delimited) CSV, not a pipe delimited version. Thanks, Dave. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
pgadmin-hackers by date: