Horton

Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Justin Pryzby <pryzby@telsasoft.com>

View thread View on postgr.es
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>
Date: 2022-11-30T02:51:13Z
Lists: pgsql-hackers 
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

Commits

  1. Stabilize pg_stat_io writes test

  2. Fix flakey pg_stat_io test

  3. Suppress more compiler warnings in new pgstats code.

  4. Suppress compiler warnings in new pgstats code.

  5. Add tests for pg_stat_io

  6. Create regress_tblspc in test_setup

  7. Add pg_stat_io view, providing more detailed IO statistics

  8. pgstat: Track more detailed relation IO statistics

  9. pgstat: Infrastructure for more detailed IO statistics

  10. doc: Fix some issues in logical replication section

  11. Manual cleanup and pgindent of pgstat and bufmgr related code

  12. Have the planner consider Incremental Sort for DISTINCT

  13. Use actual backend IDs in pg_stat_get_backend_idset() and friends.

  14. Remove redundant call to pgstat_report_wal()

  15. Add BackendType for standalone backends

  16. Initialize backend status reporting during bootstrap.