Re: pgAgent: C++ Port - Patch Review

Hi

On Fri, Sep 13, 2013 at 11:43 AM, Linreg <linreg(at)gmx(dot)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