MIME-Version: 1.0
References: <fe99e0ca-e564-480e-b865-5f0cee30bc60@tantorlabs.com>
 <CAPpHfdsTKAQqC3A48-MGQhrhfEamXZPb64w=utk7thQcOMNr7Q@mail.gmail.com>
 <320a31e6-4c91-438b-ab17-8a1d72384727@tantorlabs.com> <f202ed0f-28c3-45aa-884c-db0a78322342@tantorlabs.com>
 <fb3df2e0-2cd4-49ea-bfda-148bc43d613d@tantorlabs.com> <52e5e3aa-066a-4daf-8d65-77c34a2f0d98@tantorlabs.com>
 <6482A576-4465-4E46-A42C-F1767E8DDE30@yandex-team.ru> <CAA5RZ0s=JH4owVr=Tfr4DYpFnBrYJjKoq2d7NqdkBQKY5whiRQ@mail.gmail.com>
 <20994d10-ab44-4c6b-91bc-c4675cd2e58e@tantorlabs.com> <9D8A5BC8-A3A5-4DA9-9382-F30D343247D1@yandex-team.ru>
 <CAA5RZ0sBsajHdE_HPoJ-C5rp8QmfArd6a6yJo6bDMVpyW25E+Q@mail.gmail.com>
 <6cf6270b-e8fa-4d99-b74a-17e781cdf309@tantorlabs.com> <b40b76c3-fc54-4f11-a50f-e63cd6434560@postgrespro.ru>
 <3a27c830-b81f-4560-9774-a87d9b922fbd@tantorlabs.com> <CAA5RZ0vYu+dm3KKd5iaQ=LY0J2sjfxJG+-VCfkeVtWFuhSb-4A@mail.gmail.com>
 <cebfee9c-a894-46f1-b6bc-5bf4a0753b41@tantorlabs.com> <7AD765EF-14DE-4B3E-8282-E82A9B44DBAA@yandex-team.ru>
 <e738eac5-ad4d-454c-a079-38766e32ce52@tantorlabs.com>
In-Reply-To: <e738eac5-ad4d-454c-a079-38766e32ce52@tantorlabs.com>
From: Sami Imseih <samimseih@gmail.com>
Date: Mon, 27 Jan 2025 17:00:53 -0600
Message-ID: <CAA5RZ0s+khQ_u84qS_cAHzxVQQHK_UmWXxRBWTTex7zW1250tQ@mail.gmail.com>
Subject: Re: Sample rate added to pg_stat_statements
To: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>, Alena Rybakina <a.rybakina@postgrespro.ru>, 
	Alexander Korotkov <aekorotkov@gmail.com>, Michael Paquier <michael@paquier.xyz>, 
	Greg Sabino Mullane <htamfids@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAA5RZ0s%2BkhQ_u84qS_cAHzxVQQHK_UmWXxRBWTTex7zW1250tQ%40mail.gmail.com>
Precedence: bulk

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