Date: Tue, 29 Nov 2022 20:51:13 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Maciek Sakrejda <m.sakrejda@gmail.com>,
	Andres Freund <andres@anarazel.de>, Lukas Fittl <lukas@fittl.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Magnus Hagander <magnus@hagander.net>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20221130025113.GD24131@telsasoft.com>
References: 
 <CAAKRu_a_7JDexgYGFGeNiFfjBBkhwcPwjnkesNnTuF6hFgUY5A@mail.gmail.com>
 <CAOtHd0Aj-F1ogXiEWE4wV5U8A8a-mxS=hYwx9B3fsg57hG2zWg@mail.gmail.com>
 <CAAKRu_agmNMyMbfffGegbgFpUUTL=k3U64n_v0gLQGW5DeDkhg@mail.gmail.com>
 <20221020173101.32d2is2ldsdym7jl@awork3.anarazel.de>
 <CAOtHd0BkyGEXN6odK71=PZzK_TndbZkFstwgrtX-vA39KOpbuA@mail.gmail.com>
 <CAAKRu_YKUGKYCZrXCgtky5jx04QhvGMdkNDEN0uhBWPuVkvOpQ@mail.gmail.com>
 <CAOtHd0DU9Ymo+XQ_2Ddcacr2wS_a8-B4+AUVzJbSkDi8jkd4Xw@mail.gmail.com>
 <CAAKRu_bb8GupZOLBasf2XkHBrkj8ow8=ZOmcBOj4_G90h2TFKQ@mail.gmail.com>
 <20221123054329.GG11463@telsasoft.com>
 <CAAKRu_aioBQpF19Jkh75q7gid+FaW-JnFoGw9b-zeyhqD4w2+w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_aioBQpF19Jkh75q7gid+FaW-JnFoGw9b-zeyhqD4w2+w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221130025113.GD24131%40telsasoft.com>
Precedence: bulk

On Mon, Nov 28, 2022 at 09:08:36PM -0500, Melanie Plageman wrote:
> > +pgstat_io_op_stats_collected(BackendType bktype)
> > +{
> > +       return bktype != B_INVALID && bktype != B_ARCHIVER && bktype != B_LOGGER &&
> > +               bktype != B_WAL_RECEIVER && bktype != B_WAL_WRITER;
> >
> > Similar: I'd prefer to see this as 5 "ifs" or a "switch" to return
> > false, else return true.  But YMMV.
> 
> I don't know that separating it into multiple if statements or a switch
> would make it more clear to me or help me with debugging here.
> 
> Separately, since this is used in non-assert builds, I would like to
> ensure it is efficient. Do you know if a switch or if statements will
> be compiled to the exact same thing as this at useful optimization
> levels?

This doesn't seem like a detail worth much bother, but I did a test.

With -O2 (but not -O1 nor -Og) the assembly (gcc 9.4) is the same when
written like:

+       if (bktype == B_INVALID)
+               return false;
+       if (bktype == B_ARCHIVER)
+               return false;
+       if (bktype == B_LOGGER)
+               return false;
+       if (bktype == B_WAL_RECEIVER)
+               return false;
+       if (bktype == B_WAL_WRITER)
+               return false;
+
+       return true;

objdump --disassemble=pgstat_io_op_stats_collected src/backend/postgres_lib.a.p/utils_activity_pgstat_io_ops.c.o

0000000000000110 <pgstat_io_op_stats_collected>:
 110:   f3 0f 1e fa             endbr64 
 114:   b8 01 00 00 00          mov    $0x1,%eax
 119:   83 ff 0d                cmp    $0xd,%edi
 11c:   77 10                   ja     12e <pgstat_io_op_stats_collected+0x1e>
 11e:   b8 03 29 00 00          mov    $0x2903,%eax
 123:   89 f9                   mov    %edi,%ecx
 125:   48 d3 e8                shr    %cl,%rax
 128:   48 f7 d0                not    %rax
 12b:   83 e0 01                and    $0x1,%eax
 12e:   c3                      retq   

I was surprised, but the assembly is *not* the same when I used a switch{}.

I think it's fine to write however you want.

> > pgstat_count_io_op() has a superflous newline before "}".
> 
> I couldn't find the one you are referencing.
> Do you mind pasting in the code?

+               case IOOP_WRITE:
+                       pending_counters->writes++;
+                       break;
+       }
+ --> here <--
+}

-- 
Justin