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.

The connect/disconnect is one time for every job that will be running. That can be removed too. (move the Jobthread-SQL-action to the Mainloop)