http://www.slony.info/bugzilla/show_bug.cgi?id=175

--- Comment #15 from Steve Singer <ssinger at ca.afilias.info> 2011-03-18 07:44:48 PDT ---
Some additional comments based on the latest changes (these are mostly issues
that I just didn't catch in my first review)


1.  We also need to include support in the upgradeSchema() function to take a
2.0.x slony install and add in the new tables for monitoring

2.  In monitor thread - dbconn is never closed.  Especially in the cases where
we 'stop' the monitoring thread we should shutdown the connection (if we are
able). We also MIGHT want to think about emptying the queue at this point.  We
aren't going to empty it later and if the database connection was stuck for a
while before it timed out leading to the error the queue might have grown in
size.  

I also still wonder if we shouldn't be putting a maximum size on the queue, I
know we discussed this the other day and it started to sound complicated.  I
think
there must be a way of doing this with throwing away old entries that isn't
complicated.  If I can find some time I'll see if I can come up with something.
Until then I don't think that needs to hold this patch up.


I also bet we will get complaints from people 
"my slon has been running fine for 3 months but slon_component table stopped
updating a few weeks ago".   Where some network blimp disabled the
monitor_thread but the rest of slony continued to chug along.  Though i think
this situation is better htan slon restarting as we had before.


3. Line 189 dstring_free(monquery) before breaking out of the loop on the
error.

The discussion on how we should be searching for performance regressions in
slony is maybe outside the scope of this bug other than we should deal with
this before we release 2.1  (looking to see if any of the 2.1 changes are a
performance issue.  I have ideas on how to do this but Jan might already have a
setup for testing this).

-- 
Configure bugmail: http://www.slony.info/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.