MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 10 Nov 2023 20:32:34 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Andres Freund <andres@anarazel.de>
Cc: Michael Paquier <michael@paquier.xyz>,
 bharath.rupireddyforpostgres@gmail.com, boekewurm+postgres@gmail.com,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: Add new option 'all' to pg_stat_reset_shared()
In-Reply-To: <89C9D2DC-8239-4373-991A-18CC45D6A5FE@anarazel.de>
References: <20231104014905.oiropxau5can5fdu@awork3.anarazel.de>
 <e784c0d54c849e86d69138883d12138f@oss.nttdata.com>
 <CALj2ACVjpHfkCUWSMfpX-oyuqDCenbbduxNfQWPUBXZ0TChMrg@mail.gmail.com>
 <20231108041331.mmxtbr3cmufy4f3v@awork3.anarazel.de>
 <CALj2ACW4Fqc_m+OaavrOMEivZ5aBa24pVKvoXRTmuFECsNBfAg@mail.gmail.com>
 <ZUwguPB1rTXBK5CT@paquier.xyz>
 <62d8244b05557a91e25914c02607b493@oss.nttdata.com>
 <ZUw0_tA7g0dkTow8@paquier.xyz>
 <20231109012952.fft2i356ofljugt6@awork3.anarazel.de>
 <37bce6feca33e0c0441cd33f9eaa51d6@oss.nttdata.com>
 <ZUyKCE2l3mEf3d2Q@paquier.xyz>
 <89C9D2DC-8239-4373-991A-18CC45D6A5FE@anarazel.de>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <6c01af77007905c73751f42eea03aef5@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/6c01af77007905c73751f42eea03aef5%40oss.nttdata.com>
Precedence: bulk

On 2023-11-10 13:18, Andres Freund wrote:
> Hi,
> 
> On November 8, 2023 11:28:08 PM PST, Michael Paquier
> <michael@paquier.xyz> wrote:
>> On Thu, Nov 09, 2023 at 01:50:34PM +0900, torikoshia wrote:
>>> PGSTAT_KIND_SLRU cannot be reset by pg_stat_reset_shared(), so I feel
>>> uncomfortable to delete it all together.
>>> It might be better after pg_stat_reset_shared() has been modified to 
>>> take
>>> 'slru' as an argument, though.
>> 
>> Not sure how to feel about that, TBH, but I would not include SLRUs
>> here if we have already a separate function.
>> 
>>> I thought it would be better to reset statistics even when null is 
>>> specified
>>> so that users are not confused with the behavior of 
>>> pg_stat_reset_slru().
>>> Attached patch added pg_stat_reset_shared() in system_functions.sql 
>>> mainly
>>> for this reason.
>> 
>> I'm OK with doing what your patch does, aka do the work if the value
>> is NULL or if there's no argument given.
>> 
>> -        Resets some cluster-wide statistics counters to zero, 
>> depending on the
>> +        Resets cluster-wide statistics counters to zero, depending on 
>> the
>> 
>> This does not need to change, aka SLRUs are for example still global
>> and not included here.
>> 
>> +        If the argument is NULL or not specified, all counters shown 
>> in all
>> +        of these views are reset.
>> 
>> Missing a <literal> markup around NULL.  I know, we're not consistent
>> about that, either, but if we are tweaking the area let's be right at
>> least.  Perhaps "all the counters from the views listed above are
>> reset"?
> 
> I see no reason to not include slrus. We should never have added the
> ability to reset them individually, particularly not without a use
> case - I couldn't find one skimming some discussion. And what's the
> point in not allowing to reset them via pg_stat_reset_shared()?

When including SLRUs, do you think it's better to add 'slrus' argument 
which enables pg_stat_reset_shared() to reset all SLRUs?

As described above, since SLRUs cannot be reset by 
pg_stat_reset_shared(), I feel a bit uncomfortable to delete it all 
together.


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation