Here are some review comments for patch v79-0002.
======
General
1.
I saw that earlier in this thread Hou-san [1] and Amit [2] also seemed
to say there is not much point for this patch.
So I wanted to +1 that same opinion.
I feel this patch just adds more complexity for almost no gain:
- reducing the 'max_apply_workers_per_suibscription' seems not very
common in the first place.
- even when the GUC is reduced, at that point in time all the workers
might be in use so there may be nothing that can be immediately done.
- IIUC the excess workers (for a reduced GUC) are going to get freed
naturally anyway over time as more transactions are completed so the
pool size will reduce accordingly.
~
OTOH some refactoring parts of this patch (e.g. the new pa_stop_worker
function) look better to me. I would keep those ones but remove all
the pa_stop_idle_workers function/call.
*** NOTE: The remainder of these review comments are maybe only
relevant if you are going to keep this pa_stop_idle_workers
behaviour...
======
Commit message
2.
If the max_parallel_apply_workers_per_subscription is changed to a
lower value, try to stop free workers in the pool to keep the number of
workers lower than half of the max_parallel_apply_workers_per_subscription
SUGGESTION
If the GUC max_parallel_apply_workers_per_subscription is changed to a
lower value, try to stop unused workers to keep the pool size lower
than half of max_parallel_apply_workers_per_subscription.
======
.../replication/logical/applyparallelworker.c
3. pa_free_worker
if (winfo->serialize_changes ||
list_length(ParallelApplyWorkerPool) >
(max_parallel_apply_workers_per_subscription / 2))
{
pa_stop_worker(winfo);
return;
}
winfo->in_use = false;
winfo->serialize_changes = false;
~
IMO the above code can be more neatly written using if/else because
then there is only one return point, and there is a place to write the
explanatory comment about the else.
SUGGESTION
if (winfo->serialize_changes ||
list_length(ParallelApplyWorkerPool) >
(max_parallel_apply_workers_per_subscription / 2))
{
pa_stop_worker(winfo);
}
else
{
/* Don't stop the worker. Only mark it available for re-use. */
winfo->in_use = false;
winfo->serialize_changes = false;
}
======
src/backend/replication/logical/worker.c
4. pa_stop_idle_workers
/*
* Try to stop parallel apply workers that are not in use to keep the number of
* workers lower than half of the max_parallel_apply_workers_per_subscription.
*/
void
pa_stop_idle_workers(void)
{
List *active_workers;
ListCell *lc;
int max_applyworkers = max_parallel_apply_workers_per_subscription / 2;
if (list_length(ParallelApplyWorkerPool) <= max_applyworkers)
return;
active_workers = list_copy(ParallelApplyWorkerPool);
foreach(lc, active_workers)
{
ParallelApplyWorkerInfo *winfo = (ParallelApplyWorkerInfo *) lfirst(lc);
pa_stop_worker(winfo);
/* Recheck the number of workers. */
if (list_length(ParallelApplyWorkerPool) <= max_applyworkers)
break;
}
list_free(active_workers);
}
~
4a. function comment
SUGGESTION
Try to keep the worker pool size lower than half of the
max_parallel_apply_workers_per_subscription.
~
4b. function name
This is not stopping all idle workers, so maybe a more meaningful name
for this function is something more like "pa_reduce_workerpool"
~
4c.
IMO the "max_applyworkers" var is a misleading name. Maybe something
like "goal_poolsize" is better?
~
4d.
Maybe I misunderstand the logic for the pool, but shouldn't this be
checking the winfo->in_use flag before blindly stopping each worker?
======
src/backend/replication/logical/worker.c
5.
@@ -3630,6 +3630,13 @@ LogicalRepApplyLoop(XLogRecPtr last_received)
{
ConfigReloadPending = false;
ProcessConfigFile(PGC_SIGHUP);
+
+ /*
+ * Try to stop free workers in the pool in case the
+ * max_parallel_apply_workers_per_subscription is changed to a
+ * lower value.
+ */
+ pa_stop_idle_workers();
}
5a.
SUGGESTED COMMENT
If max_parallel_apply_workers_per_subscription is changed to a lower
value, try to reduce the worker pool to match.
~
5b.
Instead of unconditionally calling pa_stop_idle_workers, shouldn't
this code compare the value of
max_parallel_apply_workers_per_subscription before/after the
ProcessConfigFile so it only calls if the GUC was lowered?
------
[1] Hou-san -
https://www.postgresql.org/message-id/OS0PR01MB5716E527412A3481F90B4397941A9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
[2] Amit -
https://www.postgresql.org/message-id/CAA4eK1J%3D9m-VNRMHCqeG8jpX0CTn3Ciad2o4H-ogrZMDJ3tn4w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
