MIME-Version: 1.0
References: <CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>
 <20230124.172203.448918351330829200.horikyota.ntt@gmail.com>
 <20230124223512.xqp7tjgqvzqokaxq@awork3.anarazel.de> <20230125.165617.724951472557617342.horikyota.ntt@gmail.com>
 <20230208063814.boyh5ibydbfuo2uk@awork3.anarazel.de> <20230209050319.chyyup4vtq4jzobq@awork3.anarazel.de>
 <20520.1677435600@sss.pgh.pa.us> <23770.1677437567@sss.pgh.pa.us>
 <CAAKRu_Yci1Pw5qnoYdPz-FFqX-qpS0y0R_TA=rA_njACGp-SGQ@mail.gmail.com> <354645.1677511842@sss.pgh.pa.us>
In-Reply-To: <354645.1677511842@sss.pgh.pa.us>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Mon, 27 Feb 2023 14:03:16 -0500
Message-ID: <CAAKRu_bLrO9Wk4_Nuq1CpJ45RUohaq_51Lty28BF2TwjXyTd+Q@mail.gmail.com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, vignesh21@gmail.com, 
	pryzby@telsasoft.com, lukas@fittl.com, alvherre@alvh.no-ip.org, 
	magnus@hagander.net, pgsql-hackers@postgresql.org, thomas.munro@gmail.com, 
	m.sakrejda@gmail.com
Content-Type: multipart/mixed; boundary="00000000000062417c05f5b3226c"
Archived-At: <https://www.postgresql.org/message-id/CAAKRu_bLrO9Wk4_Nuq1CpJ45RUohaq_51Lty28BF2TwjXyTd%2BQ%40mail.gmail.com>
Precedence: bulk

--00000000000062417c05f5b3226c
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 27, 2023 at 10:30=E2=80=AFAM Tom Lane <tgl@sss.pgh.pa.us> wrote=
:
>
> Melanie Plageman <melanieplageman@gmail.com> writes:
> > Attached is a patch to remove the *_FIRST macros.
> > I was going to add in code to change
>
> >     for (IOObject io_object =3D 0; io_object < IOOBJECT_NUM_TYPES; io_o=
bject++)
> >     to
> >     for (IOObject io_object =3D 0; (int) io_object < IOOBJECT_NUM_TYPES=
; io_object++)
>
> I don't really like that proposal.  ISTM it's just silencing the
> messenger rather than addressing the underlying problem, namely that
> there's no guarantee that an IOObject variable can hold the value
> IOOBJECT_NUM_TYPES, which it had better do if you want the loop to
> terminate.  Admittedly it's quite unlikely that these three enums would
> grow to the point that that becomes an actual hazard for them --- but
> IMO it's still bad practice and a bad precedent for future code.

That's fair. Patch attached.

> > but then I couldn't remember why we didn't just do
>
> >     for (int io_object =3D 0; io_object < IOOBJECT_NUM_TYPES; io_object=
++)
>
> > I recall that when passing that loop variable into a function I was
> > getting a compiler warning that required me to cast the value back to a=
n
> > enum to silence it:
>
> >             pgstat_tracks_io_op(bktype, (IOObject) io_object,
> > io_context, io_op))
>
> > However, I am now unable to reproduce that warning.
> > Moreover, I see in cases like table_block_relation_size() with
> > ForkNumber, the variable i is passed with no cast to smgrnblocks().
>
> Yeah, my druthers would be to just do it the way we do comparable
> things with ForkNumber.  I don't feel like we need to invent a
> better way here.
>
> The risk of needing to cast when using the "int" loop variable
> as an enum is obviously the downside of that approach, but we have
> not seen any indication that any compilers actually do warn.
> It's interesting that you did see such a warning ... I wonder which
> compiler you were using at the time?

so, pretty much any version of clang I tried with
-Wsign-conversion produces a warning.

<source>:35:32: warning: implicit conversion changes signedness: 'int'
to 'IOOp' (aka 'enum IOOp') [-Wsign-conversion]

I didn't do the casts in the attached patch since they aren't done elsewher=
e.

- Melanie

--00000000000062417c05f5b3226c
Content-Type: text/x-patch; charset="US-ASCII"; 
	name="Change-IO-stats-enum-loop-variables-to-ints.patch"
Content-Disposition: attachment; 
	filename="Change-IO-stats-enum-loop-variables-to-ints.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_len6quek0>
X-Attachment-Id: f_len6quek0

RnJvbSAzNGZlZTRhM2QxZDEzNTNhYTM4YTk1YjNhZmMyZDMwMmE1ZjU4NmZmIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBNZWxhbmllIFBsYWdlbWFuIDxtZWxhbmllcGxhZ2VtYW5AZ21h
aWwuY29tPgpEYXRlOiBNb24sIDI3IEZlYiAyMDIzIDA4OjQ4OjExIC0wNTAwClN1YmplY3Q6IFtQ
QVRDSCB2MSAyLzJdIENoYW5nZSBJTyBzdGF0cyBlbnVtIGxvb3AgdmFyaWFibGVzIHRvIGludHMK
ClBlciBbMV0sIHVzaW5nIGFuIGVudW0gYXMgdGhlIGxvb3AgdmFyaWFibGUgd2l0aCBhIG1hY3Jv
LWRlZmluZWQKdGVybWluYXRpb24gY29uZGl0aW9uIG9mICNfZW51bV92YWx1ZXMgKyAxIGlzIG5v
dCBndWFyYW50ZWVkIHRvIGJlIHNhZmUKLSBhcyBjb21waWxlcnMgYXJlIGZyZWUgdG8gbWFrZSBl
bnVtcyBhcyBzbWFsbCBhcyBhIGNoYXIuCgpTb21lIChvbGRlcikgY29tcGlsZXJzIHdpbGwgbm90
aWNlIHRoaXMgd2hlbiBidWlsZGluZyB3aXRoCi1XdGF1dG9sb2dpY2FsLWNvbnN0YW50LW91dC1v
Zi1yYW5nZS1jb21wYXJlLgoKWzFdIGh0dHBzOi8vd3d3LnBvc3RncmVzcWwub3JnL21lc3NhZ2Ut
aWQvMzU0NjQ1LjE2Nzc1MTE4NDIlNDBzc3MucGdoLnBhLnVzCi0tLQogc3JjL2JhY2tlbmQvdXRp
bHMvYWN0aXZpdHkvcGdzdGF0X2lvLmMgfCAxMiArKysrKystLS0tLS0KIHNyYy9iYWNrZW5kL3V0
aWxzL2FkdC9wZ3N0YXRmdW5jcy5jICAgIHwgIDggKysrKy0tLS0KIDIgZmlsZXMgY2hhbmdlZCwg
MTAgaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tl
bmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0X2lvLmMgYi9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0
eS9wZ3N0YXRfaW8uYwppbmRleCBjNDc4YjEyNmZhLi5jNDE5OWQxOGM4IDEwMDY0NAotLS0gYS9z
cmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXRfaW8uYworKysgYi9zcmMvYmFja2VuZC91
dGlscy9hY3Rpdml0eS9wZ3N0YXRfaW8uYwpAQCAtMzYsMTYgKzM2LDE2IEBAIHBnc3RhdF9ia3R5
cGVfaW9fc3RhdHNfdmFsaWQoUGdTdGF0X0JrdHlwZUlPICpiYWNrZW5kX2lvLAogewogCWJvb2wJ
CWJrdHlwZV90cmFja2VkID0gcGdzdGF0X3RyYWNrc19pb19ia3R5cGUoYmt0eXBlKTsKIAotCWZv
ciAoSU9PYmplY3QgaW9fb2JqZWN0ID0gMDsgaW9fb2JqZWN0IDwgSU9PQkpFQ1RfTlVNX1RZUEVT
OyBpb19vYmplY3QrKykKKwlmb3IgKGludCBpb19vYmplY3QgPSAwOyBpb19vYmplY3QgPCBJT09C
SkVDVF9OVU1fVFlQRVM7IGlvX29iamVjdCsrKQogCXsKLQkJZm9yIChJT0NvbnRleHQgaW9fY29u
dGV4dCA9IDA7IGlvX2NvbnRleHQgPCBJT0NPTlRFWFRfTlVNX1RZUEVTOyBpb19jb250ZXh0Kysp
CisJCWZvciAoaW50IGlvX2NvbnRleHQgPSAwOyBpb19jb250ZXh0IDwgSU9DT05URVhUX05VTV9U
WVBFUzsgaW9fY29udGV4dCsrKQogCQl7CiAJCQkvKgogCQkJICogRG9uJ3QgYm90aGVyIHRyeWlu
ZyB0byBza2lwIHRvIHRoZSBuZXh0IGxvb3AgaXRlcmF0aW9uIGlmCiAJCQkgKiBwZ3N0YXRfdHJh
Y2tzX2lvX29iamVjdCgpIHdvdWxkIHJldHVybiBmYWxzZSBoZXJlLiBXZSBzdGlsbAogCQkJICog
bmVlZCB0byB2YWxpZGF0ZSB0aGF0IGVhY2ggY291bnRlciBpcyB6ZXJvIGFueXdheS4KIAkJCSAq
LwotCQkJZm9yIChJT09wIGlvX29wID0gMDsgaW9fb3AgPCBJT09QX05VTV9UWVBFUzsgaW9fb3Ar
KykKKwkJCWZvciAoaW50IGlvX29wID0gMDsgaW9fb3AgPCBJT09QX05VTV9UWVBFUzsgaW9fb3Ar
KykKIAkJCXsKIAkJCQkvKiBObyBzdGF0cywgc28gbm90aGluZyB0byB2YWxpZGF0ZSAqLwogCQkJ
CWlmIChiYWNrZW5kX2lvLT5kYXRhW2lvX29iamVjdF1baW9fY29udGV4dF1baW9fb3BdID09IDAp
CkBAIC0xMDksMTEgKzEwOSwxMSBAQCBwZ3N0YXRfZmx1c2hfaW8oYm9vbCBub3dhaXQpCiAJZWxz
ZSBpZiAoIUxXTG9ja0NvbmRpdGlvbmFsQWNxdWlyZShia3R5cGVfbG9jaywgTFdfRVhDTFVTSVZF
KSkKIAkJcmV0dXJuIHRydWU7CiAKLQlmb3IgKElPT2JqZWN0IGlvX29iamVjdCA9IDA7IGlvX29i
amVjdCA8IElPT0JKRUNUX05VTV9UWVBFUzsgaW9fb2JqZWN0KyspCisJZm9yIChpbnQgaW9fb2Jq
ZWN0ID0gMDsgaW9fb2JqZWN0IDwgSU9PQkpFQ1RfTlVNX1RZUEVTOyBpb19vYmplY3QrKykKIAl7
Ci0JCWZvciAoSU9Db250ZXh0IGlvX2NvbnRleHQgPSAwOyBpb19jb250ZXh0IDwgSU9DT05URVhU
X05VTV9UWVBFUzsgaW9fY29udGV4dCsrKQorCQlmb3IgKGludCBpb19jb250ZXh0ID0gMDsgaW9f
Y29udGV4dCA8IElPQ09OVEVYVF9OVU1fVFlQRVM7IGlvX2NvbnRleHQrKykKIAkJewotCQkJZm9y
IChJT09wIGlvX29wID0gMDsgaW9fb3AgPCBJT09QX05VTV9UWVBFUzsgaW9fb3ArKykKKwkJCWZv
ciAoaW50IGlvX29wID0gMDsgaW9fb3AgPCBJT09QX05VTV9UWVBFUzsgaW9fb3ArKykKIAkJCQli
a3R5cGVfc2hzdGF0cy0+ZGF0YVtpb19vYmplY3RdW2lvX2NvbnRleHRdW2lvX29wXSArPQogCQkJ
CQlQZW5kaW5nSU9TdGF0cy5kYXRhW2lvX29iamVjdF1baW9fY29udGV4dF1baW9fb3BdOwogCQl9
CmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC91dGlscy9hZHQvcGdzdGF0ZnVuY3MuYyBiL3NyYy9i
YWNrZW5kL3V0aWxzL2FkdC9wZ3N0YXRmdW5jcy5jCmluZGV4IDEyZWRhNGFkZTAuLmI2MWExMjM4
MmIgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9wZ3N0YXRmdW5jcy5jCisrKyBi
L3NyYy9iYWNrZW5kL3V0aWxzL2FkdC9wZ3N0YXRmdW5jcy5jCkBAIC0xMzA2LDcgKzEzMDYsNyBA
QCBwZ19zdGF0X2dldF9pbyhQR19GVU5DVElPTl9BUkdTKQogCiAJcmVzZXRfdGltZSA9IFRpbWVz
dGFtcFR6R2V0RGF0dW0oYmFja2VuZHNfaW9fc3RhdHMtPnN0YXRfcmVzZXRfdGltZXN0YW1wKTsK
IAotCWZvciAoQmFja2VuZFR5cGUgYmt0eXBlID0gMDsgYmt0eXBlIDwgQkFDS0VORF9OVU1fVFlQ
RVM7IGJrdHlwZSsrKQorCWZvciAoaW50IGJrdHlwZSA9IDA7IGJrdHlwZSA8IEJBQ0tFTkRfTlVN
X1RZUEVTOyBia3R5cGUrKykKIAl7CiAJCURhdHVtCQlia3R5cGVfZGVzYyA9IENTdHJpbmdHZXRU
ZXh0RGF0dW0oR2V0QmFja2VuZFR5cGVEZXNjKGJrdHlwZSkpOwogCQlQZ1N0YXRfQmt0eXBlSU8g
KmJrdHlwZV9zdGF0cyA9ICZiYWNrZW5kc19pb19zdGF0cy0+c3RhdHNbYmt0eXBlXTsKQEAgLTEz
MjUsMTEgKzEzMjUsMTEgQEAgcGdfc3RhdF9nZXRfaW8oUEdfRlVOQ1RJT05fQVJHUykKIAkJaWYg
KCFwZ3N0YXRfdHJhY2tzX2lvX2JrdHlwZShia3R5cGUpKQogCQkJY29udGludWU7CiAKLQkJZm9y
IChJT09iamVjdCBpb19vYmogPSAwOyBpb19vYmogPCBJT09CSkVDVF9OVU1fVFlQRVM7IGlvX29i
aisrKQorCQlmb3IgKGludCBpb19vYmogPSAwOyBpb19vYmogPCBJT09CSkVDVF9OVU1fVFlQRVM7
IGlvX29iaisrKQogCQl7CiAJCQljb25zdCBjaGFyICpvYmpfbmFtZSA9IHBnc3RhdF9nZXRfaW9f
b2JqZWN0X25hbWUoaW9fb2JqKTsKIAotCQkJZm9yIChJT0NvbnRleHQgaW9fY29udGV4dCA9IDA7
IGlvX2NvbnRleHQgPCBJT0NPTlRFWFRfTlVNX1RZUEVTOyBpb19jb250ZXh0KyspCisJCQlmb3Ig
KGludCBpb19jb250ZXh0ID0gMDsgaW9fY29udGV4dCA8IElPQ09OVEVYVF9OVU1fVFlQRVM7IGlv
X2NvbnRleHQrKykKIAkJCXsKIAkJCQljb25zdCBjaGFyICpjb250ZXh0X25hbWUgPSBwZ3N0YXRf
Z2V0X2lvX2NvbnRleHRfbmFtZShpb19jb250ZXh0KTsKIApAQCAtMTM1Nyw3ICsxMzU3LDcgQEAg
cGdfc3RhdF9nZXRfaW8oUEdfRlVOQ1RJT05fQVJHUykKIAkJCQkgKi8KIAkJCQl2YWx1ZXNbSU9f
Q09MX0NPTlZFUlNJT05dID0gSW50NjRHZXREYXR1bShCTENLU1opOwogCi0JCQkJZm9yIChJT09w
IGlvX29wID0gMDsgaW9fb3AgPCBJT09QX05VTV9UWVBFUzsgaW9fb3ArKykKKwkJCQlmb3IgKGlu
dCBpb19vcCA9IDA7IGlvX29wIDwgSU9PUF9OVU1fVFlQRVM7IGlvX29wKyspCiAJCQkJewogCQkJ
CQlpbnQJCQljb2xfaWR4ID0gcGdzdGF0X2dldF9pb19vcF9pbmRleChpb19vcCk7CiAKLS0gCjIu
MzcuMgoK
--00000000000062417c05f5b3226c--