MIME-Version: 1.0
References: <9a8f5783-c001-43ae-4287-63776a08f1bd@gmail.com>
 <20230220.120114.2218814506773492965.horikyota.ntt@gmail.com>
 <CAN55FZ2p=bW5hTPT_8LYcrKNvtcdhYNxrDgTjqJF7YNX9JSaEw@mail.gmail.com> <20230222.115019.1791643689188271652.horikyota.ntt@gmail.com>
In-Reply-To: <20230222.115019.1791643689188271652.horikyota.ntt@gmail.com>
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
Date: Wed, 22 Feb 2023 13:13:03 +0300
Message-ID: <CAN55FZ0-svc-om2F2jGMTyTgVit9kKJ7wzy4nfhWJRZjkB4fKw@mail.gmail.com>
Subject: Re: Refactor calculations to use instr_time
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: andres@anarazel.de, pgsql-hackers@postgresql.org
Content-Type: multipart/mixed; boundary="000000000000f3c92605f5472480"
Archived-At: <https://www.postgresql.org/message-id/CAN55FZ0-svc-om2F2jGMTyTgVit9kKJ7wzy4nfhWJRZjkB4fKw%40mail.gmail.com>
Precedence: bulk

--000000000000f3c92605f5472480
Content-Type: text/plain; charset="UTF-8"

Hi,

Thanks for the review.

On Wed, 22 Feb 2023 at 05:50, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> PgStat_PendingStats should be included in typedefs.list.

Done.

>
> + * Created for accumulating wal_write_time and wal_sync_time as a instr_time
> + * but instr_time can't be used as a type where it ends up on-disk
> + * because its units may change. PgStat_WalStats type is used for
> + * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
> + * accumulating intervals as a instr_time.
> + */
> +typedef struct PgStat_PendingWalStats
>
> IMHO, this comment looks somewhat off. Maybe we could try something
> like the following instead?
>
> > This struct stores wal-related durations as instr_time, which makes it
> > easier to accumulate them without requiring type conversions. Then,
> > during stats flush, they will be moved into shared stats with type
> > conversions.

Done. And I think we should write why we didn't change
PgStat_WalStats's variable types and instead created a new struct.
Maybe we can explain it in the commit description?

>
> The aim of this patch is to keep using instr_time for accumulation.
> So it seems like we could do the same refactoring for
> pgStatBlockReadTime, pgStatBlockWriteTime, pgStatActiveTime and
> pgStatTransactionIdleTime. What do you think - should we include this
> additional refactoring in the same patch or make a separate one for
> it?

I tried a bit but it seems the required changes for additional
refactoring aren't small. So, I think we can create a separate patch
for these changes.

Regards,
Nazir Bilal Yavuz
Microsoft

--000000000000f3c92605f5472480
Content-Type: application/octet-stream; 
	name="v4-0001-Refactor-instr_time-calculations.patch"
Content-Disposition: attachment; 
	filename="v4-0001-Refactor-instr_time-calculations.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lefin96h0>
X-Attachment-Id: f_lefin96h0

RnJvbSA5YzVkN2E5NmFlZDczMGU5ZDc3NDk2ODkwNzQyZjgxODhkMTA3MzQzIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBOYXppciBCaWxhbCBZYXZ1eiA8YnlhdnV6ODFAZ21haWwuY29t
PgpEYXRlOiBUaHUsIDIgRmViIDIwMjMgMTU6MDY6NDggKzAzMDAKU3ViamVjdDogW1BBVENIIHY0
XSBSZWZhY3RvciBpbnN0cl90aW1lIGNhbGN1bGF0aW9ucwoKLS0tCiBzcmMvYmFja2VuZC9hY2Nl
c3MvdHJhbnNhbS94bG9nLmMgICAgICAgfCAgNiArKy0tLQogc3JjL2JhY2tlbmQvc3RvcmFnZS9m
aWxlL2J1ZmZpbGUuYyAgICAgIHwgIDYgKystLS0KIHNyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5
L3Bnc3RhdF93YWwuYyB8IDMxICsrKysrKysrKysrKystLS0tLS0tLS0tLS0KIHNyYy9pbmNsdWRl
L3Bnc3RhdC5oICAgICAgICAgICAgICAgICAgICB8IDE3ICsrKysrKysrKysrKystCiBzcmMvdG9v
bHMvcGdpbmRlbnQvdHlwZWRlZnMubGlzdCAgICAgICAgfCAgMSArCiA1IGZpbGVzIGNoYW5nZWQs
IDM3IGluc2VydGlvbnMoKyksIDI0IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNr
ZW5kL2FjY2Vzcy90cmFuc2FtL3hsb2cuYyBiL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3hs
b2cuYwppbmRleCBmOWYwZjZkYjhkLi4zYzM1ZGMxY2EyIDEwMDY0NAotLS0gYS9zcmMvYmFja2Vu
ZC9hY2Nlc3MvdHJhbnNhbS94bG9nLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0v
eGxvZy5jCkBAIC0yMjA2LDggKzIyMDYsNyBAQCBYTG9nV3JpdGUoWExvZ3dydFJxc3QgV3JpdGVS
cXN0LCBUaW1lTGluZUlEIHRsaSwgYm9vbCBmbGV4aWJsZSkKIAkJCQkJaW5zdHJfdGltZQlkdXJh
dGlvbjsKIAogCQkJCQlJTlNUUl9USU1FX1NFVF9DVVJSRU5UKGR1cmF0aW9uKTsKLQkJCQkJSU5T
VFJfVElNRV9TVUJUUkFDVChkdXJhdGlvbiwgc3RhcnQpOwotCQkJCQlQZW5kaW5nV2FsU3RhdHMu
d2FsX3dyaXRlX3RpbWUgKz0gSU5TVFJfVElNRV9HRVRfTUlDUk9TRUMoZHVyYXRpb24pOworCQkJ
CQlJTlNUUl9USU1FX0FDQ1VNX0RJRkYoUGVuZGluZ1dhbFN0YXRzLndhbF93cml0ZV90aW1lLCBk
dXJhdGlvbiwgc3RhcnQpOwogCQkJCX0KIAogCQkJCVBlbmRpbmdXYWxTdGF0cy53YWxfd3JpdGUr
KzsKQEAgLTgyMDEsOCArODIwMCw3IEBAIGlzc3VlX3hsb2dfZnN5bmMoaW50IGZkLCBYTG9nU2Vn
Tm8gc2Vnbm8sIFRpbWVMaW5lSUQgdGxpKQogCQlpbnN0cl90aW1lCWR1cmF0aW9uOwogCiAJCUlO
U1RSX1RJTUVfU0VUX0NVUlJFTlQoZHVyYXRpb24pOwotCQlJTlNUUl9USU1FX1NVQlRSQUNUKGR1
cmF0aW9uLCBzdGFydCk7Ci0JCVBlbmRpbmdXYWxTdGF0cy53YWxfc3luY190aW1lICs9IElOU1RS
X1RJTUVfR0VUX01JQ1JPU0VDKGR1cmF0aW9uKTsKKwkJSU5TVFJfVElNRV9BQ0NVTV9ESUZGKFBl
bmRpbmdXYWxTdGF0cy53YWxfc3luY190aW1lLCBkdXJhdGlvbiwgc3RhcnQpOwogCX0KIAogCVBl
bmRpbmdXYWxTdGF0cy53YWxfc3luYysrOwpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvc3RvcmFn
ZS9maWxlL2J1ZmZpbGUuYyBiL3NyYy9iYWNrZW5kL3N0b3JhZ2UvZmlsZS9idWZmaWxlLmMKaW5k
ZXggMGE1MTYyNGRmMy4uZTU1Zjg2YjY3NSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvc3RvcmFn
ZS9maWxlL2J1ZmZpbGUuYworKysgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2ZpbGUvYnVmZmlsZS5j
CkBAIC00NjksOCArNDY5LDcgQEAgQnVmRmlsZUxvYWRCdWZmZXIoQnVmRmlsZSAqZmlsZSkKIAlp
ZiAodHJhY2tfaW9fdGltaW5nKQogCXsKIAkJSU5TVFJfVElNRV9TRVRfQ1VSUkVOVChpb190aW1l
KTsKLQkJSU5TVFJfVElNRV9TVUJUUkFDVChpb190aW1lLCBpb19zdGFydCk7Ci0JCUlOU1RSX1RJ
TUVfQUREKHBnQnVmZmVyVXNhZ2UudGVtcF9ibGtfcmVhZF90aW1lLCBpb190aW1lKTsKKwkJSU5T
VFJfVElNRV9BQ0NVTV9ESUZGKHBnQnVmZmVyVXNhZ2UudGVtcF9ibGtfcmVhZF90aW1lLCBpb190
aW1lLCBpb19zdGFydCk7CiAJfQogCiAJLyogd2UgY2hvb3NlIG5vdCB0byBhZHZhbmNlIGN1ck9m
ZnNldCBoZXJlICovCkBAIC01NDQsOCArNTQzLDcgQEAgQnVmRmlsZUR1bXBCdWZmZXIoQnVmRmls
ZSAqZmlsZSkKIAkJaWYgKHRyYWNrX2lvX3RpbWluZykKIAkJewogCQkJSU5TVFJfVElNRV9TRVRf
Q1VSUkVOVChpb190aW1lKTsKLQkJCUlOU1RSX1RJTUVfU1VCVFJBQ1QoaW9fdGltZSwgaW9fc3Rh
cnQpOwotCQkJSU5TVFJfVElNRV9BREQocGdCdWZmZXJVc2FnZS50ZW1wX2Jsa193cml0ZV90aW1l
LCBpb190aW1lKTsKKwkJCUlOU1RSX1RJTUVfQUNDVU1fRElGRihwZ0J1ZmZlclVzYWdlLnRlbXBf
YmxrX3dyaXRlX3RpbWUsIGlvX3RpbWUsIGlvX3N0YXJ0KTsKIAkJfQogCiAJCWZpbGUtPmN1ck9m
ZnNldCArPSBieXRlc3Rvd3JpdGU7CmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC91dGlscy9hY3Rp
dml0eS9wZ3N0YXRfd2FsLmMgYi9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXRfd2Fs
LmMKaW5kZXggZTg1OThiMmY0ZS4uMDk3OWQxYzI2OSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQv
dXRpbHMvYWN0aXZpdHkvcGdzdGF0X3dhbC5jCisrKyBiL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2
aXR5L3Bnc3RhdF93YWwuYwpAQCAtMjEsNyArMjEsNyBAQAogI2luY2x1ZGUgImV4ZWN1dG9yL2lu
c3RydW1lbnQuaCIKIAogCi1QZ1N0YXRfV2FsU3RhdHMgUGVuZGluZ1dhbFN0YXRzID0gezB9Owor
UGdTdGF0X1BlbmRpbmdXYWxTdGF0cyBQZW5kaW5nV2FsU3RhdHMgPSB7MH07CiAKIC8qCiAgKiBX
QUwgdXNhZ2UgY291bnRlcnMgc2F2ZWQgZnJvbSBwZ1dBTFVzYWdlIGF0IHRoZSBwcmV2aW91cyBj
YWxsIHRvCkBAIC03MCw3ICs3MCw3IEBAIGJvb2wKIHBnc3RhdF9mbHVzaF93YWwoYm9vbCBub3dh
aXQpCiB7CiAJUGdTdGF0U2hhcmVkX1dhbCAqc3RhdHNfc2htZW0gPSAmcGdTdGF0TG9jYWwuc2ht
ZW0tPndhbDsKLQlXYWxVc2FnZQlkaWZmID0gezB9OworCVdhbFVzYWdlCXdhbF91c2FnZV9kaWZm
ID0gezB9OwogCiAJQXNzZXJ0KElzVW5kZXJQb3N0bWFzdGVyIHx8ICFJc1Bvc3RtYXN0ZXJFbnZp
cm9ubWVudCk7CiAJQXNzZXJ0KHBnU3RhdExvY2FsLnNobWVtICE9IE5VTEwgJiYKQEAgLTg4LDI1
ICs4OCwyNiBAQCBwZ3N0YXRfZmx1c2hfd2FsKGJvb2wgbm93YWl0KQogCSAqIENhbGN1bGF0ZSBo
b3cgbXVjaCBXQUwgdXNhZ2UgY291bnRlcnMgd2VyZSBpbmNyZWFzZWQgYnkgc3VidHJhY3Rpbmcg
dGhlCiAJICogcHJldmlvdXMgY291bnRlcnMgZnJvbSB0aGUgY3VycmVudCBvbmVzLgogCSAqLwot
CVdhbFVzYWdlQWNjdW1EaWZmKCZkaWZmLCAmcGdXYWxVc2FnZSwgJnByZXZXYWxVc2FnZSk7Ci0J
UGVuZGluZ1dhbFN0YXRzLndhbF9yZWNvcmRzID0gZGlmZi53YWxfcmVjb3JkczsKLQlQZW5kaW5n
V2FsU3RhdHMud2FsX2ZwaSA9IGRpZmYud2FsX2ZwaTsKLQlQZW5kaW5nV2FsU3RhdHMud2FsX2J5
dGVzID0gZGlmZi53YWxfYnl0ZXM7CisJV2FsVXNhZ2VBY2N1bURpZmYoJndhbF91c2FnZV9kaWZm
LCAmcGdXYWxVc2FnZSwgJnByZXZXYWxVc2FnZSk7CiAKIAlpZiAoIW5vd2FpdCkKIAkJTFdMb2Nr
QWNxdWlyZSgmc3RhdHNfc2htZW0tPmxvY2ssIExXX0VYQ0xVU0lWRSk7CiAJZWxzZSBpZiAoIUxX
TG9ja0NvbmRpdGlvbmFsQWNxdWlyZSgmc3RhdHNfc2htZW0tPmxvY2ssIExXX0VYQ0xVU0lWRSkp
CiAJCXJldHVybiB0cnVlOwogCi0jZGVmaW5lIFdBTFNUQVRfQUNDKGZsZCkgc3RhdHNfc2htZW0t
PnN0YXRzLmZsZCArPSBQZW5kaW5nV2FsU3RhdHMuZmxkCi0JV0FMU1RBVF9BQ0Mod2FsX3JlY29y
ZHMpOwotCVdBTFNUQVRfQUNDKHdhbF9mcGkpOwotCVdBTFNUQVRfQUNDKHdhbF9ieXRlcyk7Ci0J
V0FMU1RBVF9BQ0Mod2FsX2J1ZmZlcnNfZnVsbCk7Ci0JV0FMU1RBVF9BQ0Mod2FsX3dyaXRlKTsK
LQlXQUxTVEFUX0FDQyh3YWxfc3luYyk7Ci0JV0FMU1RBVF9BQ0Mod2FsX3dyaXRlX3RpbWUpOwot
CVdBTFNUQVRfQUNDKHdhbF9zeW5jX3RpbWUpOworI2RlZmluZSBXQUxTVEFUX0FDQyhmbGQsIHZh
cl90b19hZGQpIFwKKyAJKHN0YXRzX3NobWVtLT5zdGF0cy5mbGQgKz0gdmFyX3RvX2FkZC5mbGQp
CisjZGVmaW5lIFdBTExTVEFUX0FDQ19JTlNUUl9USU1FX1RZUEUoZmxkKSBcCisJKHN0YXRzX3No
bWVtLT5zdGF0cy5mbGQgKz0gSU5TVFJfVElNRV9HRVRfTUlDUk9TRUMoUGVuZGluZ1dhbFN0YXRz
LmZsZCkpCisJV0FMU1RBVF9BQ0Mod2FsX3JlY29yZHMsIHdhbF91c2FnZV9kaWZmKTsKKwlXQUxT
VEFUX0FDQyh3YWxfZnBpLCB3YWxfdXNhZ2VfZGlmZik7CisJV0FMU1RBVF9BQ0Mod2FsX2J5dGVz
LCB3YWxfdXNhZ2VfZGlmZik7CisJV0FMU1RBVF9BQ0Mod2FsX2J1ZmZlcnNfZnVsbCwgUGVuZGlu
Z1dhbFN0YXRzKTsKKwlXQUxTVEFUX0FDQyh3YWxfd3JpdGUsIFBlbmRpbmdXYWxTdGF0cyk7CisJ
V0FMU1RBVF9BQ0Mod2FsX3N5bmMsIFBlbmRpbmdXYWxTdGF0cyk7CisJV0FMTFNUQVRfQUNDX0lO
U1RSX1RJTUVfVFlQRSh3YWxfd3JpdGVfdGltZSk7CisJV0FMTFNUQVRfQUNDX0lOU1RSX1RJTUVf
VFlQRSh3YWxfc3luY190aW1lKTsKKyN1bmRlZiBXQUxMU1RBVF9BQ0NfSU5TVFJfVElNRV9UWVBF
CiAjdW5kZWYgV0FMU1RBVF9BQ0MKIAogCUxXTG9ja1JlbGVhc2UoJnN0YXRzX3NobWVtLT5sb2Nr
KTsKZGlmZiAtLWdpdCBhL3NyYy9pbmNsdWRlL3Bnc3RhdC5oIGIvc3JjL2luY2x1ZGUvcGdzdGF0
LmgKaW5kZXggZGI5Njc1ODg0Zi4uODJkZDdkNjQxNiAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUv
cGdzdGF0LmgKKysrIGIvc3JjL2luY2x1ZGUvcGdzdGF0LmgKQEAgLTQ0NSw2ICs0NDUsMjEgQEAg
dHlwZWRlZiBzdHJ1Y3QgUGdTdGF0X1dhbFN0YXRzCiAJVGltZXN0YW1wVHogc3RhdF9yZXNldF90
aW1lc3RhbXA7CiB9IFBnU3RhdF9XYWxTdGF0czsKIAorLyoKKyAqIFRoaXMgc3RydWN0IHN0b3Jl
cyB3YWwtcmVsYXRlZCBkdXJhdGlvbnMgYXMgaW5zdHJfdGltZSwgd2hpY2ggbWFrZXMgaXQKKyAq
IGVhc2llciB0byBhY2N1bXVsYXRlIHRoZW0gd2l0aG91dCByZXF1aXJpbmcgdHlwZSBjb252ZXJz
aW9ucy4gVGhlbiwKKyAqIGR1cmluZyBzdGF0cyBmbHVzaCwgdGhleSB3aWxsIGJlIG1vdmVkIGlu
dG8gc2hhcmVkIHN0YXRzIHdpdGggdHlwZQorICogY29udmVyc2lvbnMuCisgKi8KK3R5cGVkZWYg
c3RydWN0IFBnU3RhdF9QZW5kaW5nV2FsU3RhdHMKK3sKKwlQZ1N0YXRfQ291bnRlciB3YWxfYnVm
ZmVyc19mdWxsOworCVBnU3RhdF9Db3VudGVyIHdhbF93cml0ZTsKKwlQZ1N0YXRfQ291bnRlciB3
YWxfc3luYzsKKwlpbnN0cl90aW1lIHdhbF93cml0ZV90aW1lOworCWluc3RyX3RpbWUgd2FsX3N5
bmNfdGltZTsKK30gUGdTdGF0X1BlbmRpbmdXYWxTdGF0czsKKwogCiAvKgogICogRnVuY3Rpb25z
IGluIHBnc3RhdC5jCkBAIC03NTgsNyArNzczLDcgQEAgZXh0ZXJuIFBHRExMSU1QT1JUIFNlc3Np
b25FbmRUeXBlIHBnU3RhdFNlc3Npb25FbmRDYXVzZTsKICAqLwogCiAvKiB1cGRhdGVkIGRpcmVj
dGx5IGJ5IGJhY2tlbmRzIGFuZCBiYWNrZ3JvdW5kIHByb2Nlc3NlcyAqLwotZXh0ZXJuIFBHRExM
SU1QT1JUIFBnU3RhdF9XYWxTdGF0cyBQZW5kaW5nV2FsU3RhdHM7CitleHRlcm4gUEdETExJTVBP
UlQgUGdTdGF0X1BlbmRpbmdXYWxTdGF0cyBQZW5kaW5nV2FsU3RhdHM7CiAKIAogI2VuZGlmCQkJ
CQkJCS8qIFBHU1RBVF9IICovCmRpZmYgLS1naXQgYS9zcmMvdG9vbHMvcGdpbmRlbnQvdHlwZWRl
ZnMubGlzdCBiL3NyYy90b29scy9wZ2luZGVudC90eXBlZGVmcy5saXN0CmluZGV4IDIyZWE0MmMx
NmIuLjVkMjhhYTFjYTkgMTAwNjQ0Ci0tLSBhL3NyYy90b29scy9wZ2luZGVudC90eXBlZGVmcy5s
aXN0CisrKyBiL3NyYy90b29scy9wZ2luZGVudC90eXBlZGVmcy5saXN0CkBAIC0yMDQ1LDYgKzIw
NDUsNyBAQCBQZ1N0YXRfS2luZAogUGdTdGF0X0tpbmRJbmZvCiBQZ1N0YXRfTG9jYWxTdGF0ZQog
UGdTdGF0X1BlbmRpbmdEcm9wcGVkU3RhdHNJdGVtCitQZ1N0YXRfUGVuZGluZ1dhbFN0YXRzCiBQ
Z1N0YXRfU0xSVVN0YXRzCiBQZ1N0YXRfU2htZW1Db250cm9sCiBQZ1N0YXRfU25hcHNob3QKLS0g
CjIuMjUuMQoK
--000000000000f3c92605f5472480--