At 20:56 23/04/2008, Christopher Browne wrote:
>I don't see any reason in the code why limiting the number of events
>processed should break anything.  I think I'd want to set the limit
>based on a configuration parameter, but at first blush, the following
>seems reasonable:
>
>Index: remote_listen.c
>===================================================================
>RCS file: /home/cvsd/slony1/slony1-engine/src/slon/remote_listen.c,v
>retrieving revision 1.40
>diff -c -u -r1.40 remote_listen.c
>cvs diff: conflicting specifications of output style
>--- remote_listen.c     6 Feb 2008 20:20:50 -0000       1.40
>+++ remote_listen.c     23 Apr 2008 18:29:14 -0000
>@@ -1,4 +1,4 @@
>-/* ----------------------------------------------------------------------
>+* ----------------------------------------------------------------------
>   * remote_listen.c
>   *
>   *     Implementation of the thread listening for events on
>@@ -697,7 +697,7 @@
>         {
>                 slon_appendquery(&query, ")");
>         }
>-       slon_appendquery(&query, " order by e.ev_origin, e.ev_seqno");
>+       slon_appendquery(&query, " order by e.ev_origin, e.ev_seqno 
>limit 2000");
>
>         rtcfg_unlock();


I've been running slon with a similar change for a little while 
(finally catching up on a huge backlog) and it seems not to have 
caused any problems. But indeed a configuration parameter may be a good idea.

>- Every time a message is added to the queue in remote_listen.c, we
>   add to the counter
>
>- Every time a message is processed from the queue in remote_worker.c,
>   we decrement the counter
>
>- In remote_listen.c, any time the size of the queue is larger than
>   "os_event_threshold" (defaults to > the LIMIT used in the query in
>   remote_listen.c), then we sleep for "os_event_sleep" milliseconds
>   before processing another iteration of the "event search loop."

Sounds good. There could be a check that keeps the thread in a sleep 
loop until the counter comes below said threshold as well. Or the 
limit in the select could be (threshold - count). Obviously if 
count > threshold we want to skip the select and go back to sleep 
(and of course care must be taken with the poll/listen switches).

>Alternatively, this could get more sophisticated, with some extra
>config parms:
>
>  * os_event_limit            - How many events to pull at a time,
>                                and the threshold for further stuff
>  * os_event_initialsleep     - If os_events > limit, then,
>                                initially, sleep this many ms
>  * os_event_increment        - When os_events continues to be >
>                                os_event_limit, add this to the
>                                sleep time
>  * os_event_maxsleep         - Don't let sleep time exceed this

Don't know if we need that much "complexity"? I think the above logic 
("only get as much as we need") might be simpler and requires less 
tuning (and less default values to pick!).

> > I also found out that setting desired_sync_time to 0 and increasing
> > significantly sync_group_maxsize helps a lot when catching up. Is
> > there a specific reason to have a low default value for this? Since
> > it's bounded by the number of available events anyway, I'm not sure
> > how low values actually help anything -- at least when
> > desired_sync_time=0.
>
>There is a reason, if you're running log shipping; you might want to
>be sure that each SYNC is kept separate, so that you could most
>closely associate the set of data with its SYNC time.

So it should be either 1 or a relatively large value, then? Not sure 
the current default makes much sense? But I haven't had much 
experience with this other than watching slon trying to cope with 
this backlog :-( Not sure how things work out in a "normal" situation.

Also, something stupid: the code has:

max_sync = ((last_sync_group_size * 200) / 100) + 1;

I'm quite sure this can be simplified as:

max_sync = last_sync_group_size * 2 + 1;

without changing the result (actually the compiler probably does it 
already). Either that or the intended result got lost somewhere :-)

>For sure, the "desired_sync_time" is only an approximation.  It has
>the implicit assumption that run time time for a set of SYNCs is be
>roughly proportional to the number of SYNCs, which isn't always true.
>
>Any policy that is applied here is necessarily an approximation, so I
>don't know that it is too likely to see *huge* improvements from a
>substitute policy.  If you can describe another that is readily coded,
>I'll certainly listen :-).

I don't have any good suggestions for that yet, I've been doing the 
catching up with desired_sync_time = 0 so that code is effectively 
disabled :-) One possible option, though, would be to split the "time 
for first fetch" from the rest, and maybe consider that as a 
"constant" in the computation. I.e. consider that:

estimated_times_it_takes_to_handle_n_syncs = time_for_first_fetch + n 
* time_for_remainder_of_last_run / number_of_syncs_in_last_run

rather than n * time_of_last_run / number_of_syncs_in_last_run as I 
believe it is right now? I'm wild-guessing here :-)

Jacques.