Date: Wed, 15 Dec 2021 16:38:52 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Magnus Hagander <magnus@hagander.net>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Lukas Fittl <lukas@fittl.com>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20211215223852.GV17618@telsasoft.com>
References: <20211008175620.nspjy6wwmzvyzkrg@alap3.anarazel.de>
 <CAAKRu_YJMHJ66-ZB86zFCH=Cq4+5w2gnJ14bu8oyZec0C39Dcg@mail.gmail.com>
 <20211019192931.eeeohw6yrvqe6bol@alap3.anarazel.de>
 <CAAKRu_Yeg+vh6SHNEo1+=O7e-BPX35cU0XQM=YwQRnkFyv_y+w@mail.gmail.com>
 <20211119164952.mlw7ecj4xxliuuut@alap3.anarazel.de>
 <CAAKRu_bCavNULX2BUM1Ankr=Hc5ySbmMJCGnHCFZBbM=yMvFLA@mail.gmail.com>
 <20211125011559.GC17618@telsasoft.com>
 <CAAKRu_ZLE50yXXHTBEjddLfvR9fw=S79Thjzd_yCfvzkDmA7CA@mail.gmail.com>
 <20211202040653.GM17618@telsasoft.com>
 <CAAKRu_aaq33UnG4TXq3S-OSXGWj1QGf0sU+ECH4tNwGFNERkZA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_aaq33UnG4TXq3S-OSXGWj1QGf0sU+ECH4tNwGFNERkZA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211215223852.GV17618%40telsasoft.com>
Precedence: bulk

On Fri, Dec 03, 2021 at 03:02:24PM -0500, Melanie Plageman wrote:
> Thanks again! I really appreciate the thorough review.
> 
> I have combined responses to all three of your emails below.
> Let me know if it is more confusing to do it this way.

One email is better than three - I'm just not a model citizen ;)

Thanks for updating the patch.  I checked that all my previous review comments
were addressed (except for the part about passing the 3D array to a function -
I know that technically the pointer is being passed).

+int backend_type_get_idx(BackendType backend_type)                                                                                                                                               
+BackendType idx_get_backend_type(int idx)                                                                                                                                                        

=> I think it'd be desirable for these to be either static functions (which
won't work for your needs) or macros, or inline functions in the header.

-       if (strcmp(target, "archiver") == 0)                                                                                                                                                      
+       pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_RESETSHAREDCOUNTER);                                                                                                                            
+       if (strcmp(target, "buffers") == 0)                                                                                                                                                       

=> This should be added in alphabetical order.  Which is unimportant, but it
will also makes the patch 2 lines shorter.  The doc patch should also be in
order.

+                * Don't count dead backends. They will be added below There are no                                                                                                               

=> Missing a period.

-- 
Justin