Re: pgAgent: C++ Port - Patch Review

Hi Dave,

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

Sorry. Sorry. :(
i corrected this. Done

> 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.
okay. i change this feature to configurable behaviour.
By default logging will be as before.
can i add an commandline parameter for this?
is it than acceptable?

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

Okay, thats a problem.
I use features from c++11 standard like "atomic" and "thread"
My suggestion:
the Pure-C++-Version is is for newer OS / Compiler suits and not backward compatible
(relating compiler / c++ version)
Is the way a possible?

> > - 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.
it is very time consuming to keep this feature for c++-port.

please tell me for my understanding:
In the moment you reuse a connection object. okay.
But how can you use it for an another host / user combination<ß The same object.
Example
Job 1 - step 1: connect to User: x Host A
Job 1 - step 2: connect to User: x Host B
if you use the same connection object, the database log has a connect/disconnect
entry.

Job 1 - step 1: connect to User: x Host A
Job 1 - step 2: connect to User: x Host A
And when the same Host/User combination is for a reused connection:
How make you a logical reset for the database server?

In the last scenario can i make a reusing too. With move the Jobthread-SQL-action to
the Mainloop i can reduce the needed connection count to 1xJob count + 1 (service
connect). in my implementation i need at the moment 2xJobcount+1 connection
objects.