Re: pgAgent: C++ Port - Patch Review

On Fri, Sep 13, 2013 at 4:57 PM, Linreg <linreg(at)gmx(dot)net> wrote:

> **
>
> 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?
>

I think so - but please do that as a separate patch. We don't like to mixup
multiple changes in the same patch/commit as it's harder to find issues or
review the history in the future.

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

Not really, as it requires maintenance of 2 code branches until we can
completely deprecate the old wxWidgets code. That's why PostgreSQL itself
is extremely conservative about what they'll support. We're not nearly as
bad as that, but we do need to carry on supporting RHEL 5 and similarly
aged OSs for a while.

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

It doesn't reuse it for a different host/port (or even user/database). But
consider a user that has a very frequently executed job on the same
database.

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

I don't see how you would do that, unless you used a pool which must have a
mutex to protect it from concurrent accesses. Unless I'm misunderstanding
what you mean.

I'm not entirely convinced that we do need this in pgAgent - the scenario I
mentioned before is primarily in a piece of code derived from pgAgent, but
I do see situations where pooling may be needed here. Does anyone else have
an opinion on this?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company