Hi,
I have a few comments about the patch.
1/
tests are missing for pg_stat_statements.track=all and with a sample
rate < 1 applied.
Can we add tests for this case?
2/
This declaration:
+static bool current_query_sampled = false;
should be moved near the declaration of nesting_level,
the other local variable.
3/
I do not really like the calls to update_current_query_sampled()
the way there are, and I think the pgss_enabled should encapsulate
the new sample rate check as well.
My thoughts are:
1/ change the name of the function from update_current_query_sampled
to current_query_sampled
Also, the static bool should be called is_query_sampled or something like that.
The function should also allow you to skip the sample rate check, such as if
called from pgss_post_parse_analyze.
We can also remove the IsParallelWorker() check in this case, since that
is done in pgss_enabled.
something like this is what I am thinking:
static bool
current_query_sampled(bool skip)
{
if (skip)
return true;
if (nesting_level == 0)
{
is_query_sampled = pgss_sample_rate != 0.0 &&
(pgss_sample_rate == 1.0 ||
pg_prng_double(&pg_global_prng_state) <= pgss_sample_rate);
}
return is_query_sampled;
}
#define pgss_enabled(level, skip_sampling_check) \
(!IsParallelWorker() && \
(pgss_track == PGSS_TRACK_ALL || \
(pgss_track == PGSS_TRACK_TOP && (level) == 0)) && \
current_query_sampled(skip_sampling_check))
What do you think?
4/ Now we have pg_stat_statements.track = "off" which is effectively
the same thing as pg_stat_statements.sample_rate = "0". Does this need
to be called out in docs?
Regards,
Sami
