public inbox for [email protected]  

help / color / mirror / Atom feed

From: Sami Imseih <[email protected]>
To: Ilia Evdokimov <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Alena Rybakina <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Greg Sabino Mullane <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Sample rate added to pg_stat_statements
Date: Mon, 27 Jan 2025 17:00:53 -0600
Message-ID: <CAA5RZ0s+khQ_u84qS_cAHzxVQQHK_UmWXxRBWTTex7zW1250tQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAPpHfdsTKAQqC3A48-MGQhrhfEamXZPb64w=utk7thQcOMNr7Q@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAA5RZ0s=JH4owVr=Tfr4DYpFnBrYJjKoq2d7NqdkBQKY5whiRQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CAA5RZ0sBsajHdE_HPoJ-C5rp8QmfArd6a6yJo6bDMVpyW25E+Q@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAA5RZ0vYu+dm3KKd5iaQ=LY0J2sjfxJG+-VCfkeVtWFuhSb-4A@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (58+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Sample rate added to pg_stat_statements
  In-Reply-To: <CAA5RZ0s+khQ_u84qS_cAHzxVQQHK_UmWXxRBWTTex7zW1250tQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox