Re: Logging parallel worker draught

On 28.08.2024 15:58, Benoit Lobréau wrote:
> Hi,
>
> Here is a new version of the patch. Sorry for the long delay, I was 
> hit by a motivation drought and was quite busy otherwise.
>
> The guc is now called `log_parallel_workers` and has three possible 
> values:
>
> * "none": disables logging
> * "all": logs parallel worker info for all parallel queries or utilities
> * "failure": logs only when the number of parallel workers planned 
> couldn't be reached.
>
> For this, I added several members to the EState struct.
>
> Each gather node / gather merge node updates the values and the 
> offending queries are displayed during standard_ExecutorEnd.
>
> For CREATE INDEX / REINDEX on btree and brin, I check the parallel 
> context struct (pcxt) during _bt_end_parallel() or 
> _brin_end_parallel() and display a log message when needed.
>
> For vacuum, I do the same in parallel_vacuum_end().
>
> I added some information to the error message for parallel queries as 
> an experiment. I find it useful, but it can be removed, if you re not 
> convinced.
>
> 2024-08-27 15:59:11.089 CEST [54585] LOG:  1 parallel nodes planned (1 
> obtained all their workers, 0 obtained none), 2 workers planned (2 
> workers launched)
> 2024-08-27 15:59:11.089 CEST [54585] STATEMENT:  EXPLAIN (ANALYZE)
>         SELECT i, avg(j) FROM test_pql GROUP BY i;
>
> 2024-08-27 15:59:14.006 CEST [54585] LOG:  2 parallel nodes planned (0 
> obtained all their workers, 1 obtained none), 4 workers planned (1 
> workers launched)
> 2024-08-27 15:59:14.006 CEST [54585] STATEMENT:  EXPLAIN (ANALYZE)
>         SELECT i, avg(j) FROM test_pql GROUP BY i
>         UNION
>         SELECT i, avg(j) FROM test_pql GROUP BY i;
>
> For CREATE INDEX / REDINDEX / VACUUMS:
>
> 2024-08-27 15:58:59.769 CEST [54521] LOG:  1 workers planned (0 
> workers launched)
> 2024-08-27 15:58:59.769 CEST [54521] STATEMENT:  REINDEX TABLE test_pql;
>
> Do you think this is better?
>
> I am not sure if a struct is needed to store the es_nworkers* and if 
> the modification I did to parallel.h is ok.
>
> Thanks to: Jehan-Guillaume de Rorthais, Guillaume Lelarge and Franck 
> Boudehen for the help and motivation boost. 

Hi!

I think it's a good idea to log this. I suggest some changes that might 
improve your patch.

1. I think you should rewrite the first statement of the documentation 
about parameter as follows:

Controls a log message about the number of workers produced during an 
execution of the parallel query or utility statement.


2. I think you can transfer nworkers_to_launch and nworkers_launched 
vacuum parameters in the ParallelContext struct.

3. I think you should write the logging check condition in an 
independent function and provide necessary parameters for that. To be 
honest if the parameters weren't stored in a different struct for 
parallel queries, I would have put it in a macro.

I attached the diff file including my proposals.


-- 
Regards,
Alena Rybakina
Postgres Professional