Content-Type: multipart/mixed; boundary="------------bznyIeA1GF2EfYLd5FwvujdK"
Message-ID: <9a8f5783-c001-43ae-4287-63776a08f1bd@gmail.com>
Date: Fri, 17 Feb 2023 13:53:36 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: Refactor calculations to use instr_time
Content-Language: en-US
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <1feedb83-7aa9-cb4b-5086-598349d3f555@gmail.com>
 <20230216161322.zg2k7ytvqiiqmwgc@awork3.anarazel.de>
From: Nazir Bilal Yavuz <byavuz81@gmail.com>
In-Reply-To: <20230216161322.zg2k7ytvqiiqmwgc@awork3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/9a8f5783-c001-43ae-4287-63776a08f1bd%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------bznyIeA1GF2EfYLd5FwvujdK
Content-Type: multipart/alternative;
 boundary="------------vbLhLUDptgEVEbKs7k3IjTPN"

--------------vbLhLUDptgEVEbKs7k3IjTPN
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi,


On 2/16/23 19:13, Andres Freund wrote:
>> +#define WALSTAT_ACC(fld, var_to_add) \
>> + 	(stats_shmem->stats.fld += var_to_add.fld)
>> +#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
>> +	(stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
>> +	WALSTAT_ACC(wal_records, diff);
>> +	WALSTAT_ACC(wal_fpi, diff);
>> +	WALSTAT_ACC(wal_bytes, diff);
>> +	WALSTAT_ACC(wal_buffers_full, PendingWalStats);
>> +	WALSTAT_ACC(wal_write, PendingWalStats);
>> +	WALSTAT_ACC(wal_sync, PendingWalStats);
>> +	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
>> +	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
>>   #undef WALSTAT_ACC
>> -
>>   	LWLockRelease(&stats_shmem->lock);
> WALSTAT_ACC is undefined, but WALLSTAT_ACC_INSTR_TIME_TYPE isn't.
>
> I'd not remove the newline before LWLockRelease().
>
>
>>   	/*
>> diff --git a/src/include/pgstat.h b/src/include/pgstat.h
>> index db9675884f3..295c5eabf38 100644
>> --- a/src/include/pgstat.h
>> +++ b/src/include/pgstat.h
>> @@ -445,6 +445,21 @@ typedef struct PgStat_WalStats
>>   	TimestampTz stat_reset_timestamp;
>>   } PgStat_WalStats;
>>   
>> +/* Created for accumulating wal_write_time and wal_sync_time as a
>> instr_time
> Minor code-formatting point: In postgres we don't put code in the same line as
> a multi-line comment starting with the /*. So either
>
> /* single line comment */
> or
> /*
>   * multi line
>   * comment
>   */


Thanks for the review. I updated the patch.


Regards,
Nazir Bilal Yavuz
Microsoft


--------------vbLhLUDptgEVEbKs7k3IjTPN
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><font face="monospace">Hi,</font></p>
    <p><font face="monospace"><br>
      </font></p>
    <div class="moz-cite-prefix"><font face="monospace">On 2/16/23
        19:13, Andres Freund wrote:</font>
    </div>
    <blockquote type="cite"
      cite="mid:20230216161322.zg2k7ytvqiiqmwgc@awork3.anarazel.de">
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap="">+#define WALSTAT_ACC(fld, var_to_add) \
+ 	(stats_shmem-&gt;stats.fld += var_to_add.fld)
+#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
+	(stats_shmem-&gt;stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+	WALSTAT_ACC(wal_records, diff);
+	WALSTAT_ACC(wal_fpi, diff);
+	WALSTAT_ACC(wal_bytes, diff);
+	WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+	WALSTAT_ACC(wal_write, PendingWalStats);
+	WALSTAT_ACC(wal_sync, PendingWalStats);
+	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
+	WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
 #undef WALSTAT_ACC
-
 	LWLockRelease(&amp;stats_shmem-&gt;lock);
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
WALSTAT_ACC is undefined, but WALLSTAT_ACC_INSTR_TIME_TYPE isn't.

I'd not remove the newline before LWLockRelease().


</pre>
      <blockquote type="cite">
        <pre class="moz-quote-pre" wrap=""> 	/*
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f3..295c5eabf38 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -445,6 +445,21 @@ typedef struct PgStat_WalStats
 	TimestampTz stat_reset_timestamp;
 } PgStat_WalStats;
 
+/* Created for accumulating wal_write_time and wal_sync_time as a
instr_time
</pre>
      </blockquote>
      <pre class="moz-quote-pre" wrap="">
Minor code-formatting point: In postgres we don't put code in the same line as
a multi-line comment starting with the /*. So either

/* single line comment */
or
/*
 * multi line
 * comment
 */</pre>
    </blockquote>
    <p><font face="monospace"><br>
      </font></p>
    <p><font face="monospace">Thanks for the review. I updated the
        patch.</font></p>
    <p><font face="monospace"><br>
      </font></p>
    <p><font face="monospace">Regards,<br>
        Nazir Bilal Yavuz<br>
        Microsoft </font><br>
    </p>
    <p><br>
    </p>
    <p><br>
    </p>
  </body>
</html>

--------------vbLhLUDptgEVEbKs7k3IjTPN--
--------------bznyIeA1GF2EfYLd5FwvujdK
Content-Type: text/x-diff; charset=UTF-8;
 name="v2-0001-Refactor-instr_time-calculations.patch"
Content-Disposition: attachment;
 filename="v2-0001-Refactor-instr_time-calculations.patch"
Content-Transfer-Encoding: base64

RnJvbSBlMzcyM2FjYThiNzliMDcxOTA4MzRiMGNmZDE0NDBkYmJmNzA2ODYyIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBOYXppciBCaWxhbCBZYXZ1eiA8YnlhdnV6ODFAZ21h
aWwuY29tPgpEYXRlOiBUaHUsIDIgRmViIDIwMjMgMTU6MDY6NDggKzAzMDAKU3ViamVjdDog
W1BBVENIIHYyXSBSZWZhY3RvciBpbnN0cl90aW1lIGNhbGN1bGF0aW9ucwoKLS0tCiBzcmMv
YmFja2VuZC9hY2Nlc3MvdHJhbnNhbS94bG9nLmMgICAgICAgfCAgNiArKy0tLS0KIHNyYy9i
YWNrZW5kL3N0b3JhZ2UvZmlsZS9idWZmaWxlLmMgICAgICB8ICA2ICsrLS0tLQogc3JjL2Jh
Y2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0X3dhbC5jIHwgMjcgKysrKysrKysrKysrKy0t
LS0tLS0tLS0tLQogc3JjL2luY2x1ZGUvcGdzdGF0LmggICAgICAgICAgICAgICAgICAgIHwg
MTggKysrKysrKysrKysrKysrKy0KIDQgZmlsZXMgY2hhbmdlZCwgMzUgaW5zZXJ0aW9ucygr
KSwgMjIgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvYWNjZXNzL3Ry
YW5zYW0veGxvZy5jIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0veGxvZy5jCmluZGV4
IGY5ZjBmNmRiOGQxLi4zYzM1ZGMxY2EyMyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvYWNj
ZXNzL3RyYW5zYW0veGxvZy5jCisrKyBiL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3hs
b2cuYwpAQCAtMjIwNiw4ICsyMjA2LDcgQEAgWExvZ1dyaXRlKFhMb2d3cnRScXN0IFdyaXRl
UnFzdCwgVGltZUxpbmVJRCB0bGksIGJvb2wgZmxleGlibGUpCiAJCQkJCWluc3RyX3RpbWUJ
ZHVyYXRpb247CiAKIAkJCQkJSU5TVFJfVElNRV9TRVRfQ1VSUkVOVChkdXJhdGlvbik7Ci0J
CQkJCUlOU1RSX1RJTUVfU1VCVFJBQ1QoZHVyYXRpb24sIHN0YXJ0KTsKLQkJCQkJUGVuZGlu
Z1dhbFN0YXRzLndhbF93cml0ZV90aW1lICs9IElOU1RSX1RJTUVfR0VUX01JQ1JPU0VDKGR1
cmF0aW9uKTsKKwkJCQkJSU5TVFJfVElNRV9BQ0NVTV9ESUZGKFBlbmRpbmdXYWxTdGF0cy53
YWxfd3JpdGVfdGltZSwgZHVyYXRpb24sIHN0YXJ0KTsKIAkJCQl9CiAKIAkJCQlQZW5kaW5n
V2FsU3RhdHMud2FsX3dyaXRlKys7CkBAIC04MjAxLDggKzgyMDAsNyBAQCBpc3N1ZV94bG9n
X2ZzeW5jKGludCBmZCwgWExvZ1NlZ05vIHNlZ25vLCBUaW1lTGluZUlEIHRsaSkKIAkJaW5z
dHJfdGltZQlkdXJhdGlvbjsKIAogCQlJTlNUUl9USU1FX1NFVF9DVVJSRU5UKGR1cmF0aW9u
KTsKLQkJSU5TVFJfVElNRV9TVUJUUkFDVChkdXJhdGlvbiwgc3RhcnQpOwotCQlQZW5kaW5n
V2FsU3RhdHMud2FsX3N5bmNfdGltZSArPSBJTlNUUl9USU1FX0dFVF9NSUNST1NFQyhkdXJh
dGlvbik7CisJCUlOU1RSX1RJTUVfQUNDVU1fRElGRihQZW5kaW5nV2FsU3RhdHMud2FsX3N5
bmNfdGltZSwgZHVyYXRpb24sIHN0YXJ0KTsKIAl9CiAKIAlQZW5kaW5nV2FsU3RhdHMud2Fs
X3N5bmMrKzsKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3N0b3JhZ2UvZmlsZS9idWZmaWxl
LmMgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2ZpbGUvYnVmZmlsZS5jCmluZGV4IDBhNTE2MjRk
ZjNiLi5lNTVmODZiNjc1ZSAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvc3RvcmFnZS9maWxl
L2J1ZmZpbGUuYworKysgYi9zcmMvYmFja2VuZC9zdG9yYWdlL2ZpbGUvYnVmZmlsZS5jCkBA
IC00NjksOCArNDY5LDcgQEAgQnVmRmlsZUxvYWRCdWZmZXIoQnVmRmlsZSAqZmlsZSkKIAlp
ZiAodHJhY2tfaW9fdGltaW5nKQogCXsKIAkJSU5TVFJfVElNRV9TRVRfQ1VSUkVOVChpb190
aW1lKTsKLQkJSU5TVFJfVElNRV9TVUJUUkFDVChpb190aW1lLCBpb19zdGFydCk7Ci0JCUlO
U1RSX1RJTUVfQUREKHBnQnVmZmVyVXNhZ2UudGVtcF9ibGtfcmVhZF90aW1lLCBpb190aW1l
KTsKKwkJSU5TVFJfVElNRV9BQ0NVTV9ESUZGKHBnQnVmZmVyVXNhZ2UudGVtcF9ibGtfcmVh
ZF90aW1lLCBpb190aW1lLCBpb19zdGFydCk7CiAJfQogCiAJLyogd2UgY2hvb3NlIG5vdCB0
byBhZHZhbmNlIGN1ck9mZnNldCBoZXJlICovCkBAIC01NDQsOCArNTQzLDcgQEAgQnVmRmls
ZUR1bXBCdWZmZXIoQnVmRmlsZSAqZmlsZSkKIAkJaWYgKHRyYWNrX2lvX3RpbWluZykKIAkJ
ewogCQkJSU5TVFJfVElNRV9TRVRfQ1VSUkVOVChpb190aW1lKTsKLQkJCUlOU1RSX1RJTUVf
U1VCVFJBQ1QoaW9fdGltZSwgaW9fc3RhcnQpOwotCQkJSU5TVFJfVElNRV9BREQocGdCdWZm
ZXJVc2FnZS50ZW1wX2Jsa193cml0ZV90aW1lLCBpb190aW1lKTsKKwkJCUlOU1RSX1RJTUVf
QUNDVU1fRElGRihwZ0J1ZmZlclVzYWdlLnRlbXBfYmxrX3dyaXRlX3RpbWUsIGlvX3RpbWUs
IGlvX3N0YXJ0KTsKIAkJfQogCiAJCWZpbGUtPmN1ck9mZnNldCArPSBieXRlc3Rvd3JpdGU7
CmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXRfd2FsLmMg
Yi9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXRfd2FsLmMKaW5kZXggZTg1OThi
MmY0ZTAuLmFiMjg2OWEwZTI0IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC91dGlscy9hY3Rp
dml0eS9wZ3N0YXRfd2FsLmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdz
dGF0X3dhbC5jCkBAIC0yMSw3ICsyMSw3IEBACiAjaW5jbHVkZSAiZXhlY3V0b3IvaW5zdHJ1
bWVudC5oIgogCiAKLVBnU3RhdF9XYWxTdGF0cyBQZW5kaW5nV2FsU3RhdHMgPSB7MH07CitQ
Z1N0YXRfUGVuZGluZ1dhbFVzYWdlIFBlbmRpbmdXYWxTdGF0cyA9IHswfTsKIAogLyoKICAq
IFdBTCB1c2FnZSBjb3VudGVycyBzYXZlZCBmcm9tIHBnV0FMVXNhZ2UgYXQgdGhlIHByZXZp
b3VzIGNhbGwgdG8KQEAgLTg5LDI0ICs4OSwyNSBAQCBwZ3N0YXRfZmx1c2hfd2FsKGJvb2wg
bm93YWl0KQogCSAqIHByZXZpb3VzIGNvdW50ZXJzIGZyb20gdGhlIGN1cnJlbnQgb25lcy4K
IAkgKi8KIAlXYWxVc2FnZUFjY3VtRGlmZigmZGlmZiwgJnBnV2FsVXNhZ2UsICZwcmV2V2Fs
VXNhZ2UpOwotCVBlbmRpbmdXYWxTdGF0cy53YWxfcmVjb3JkcyA9IGRpZmYud2FsX3JlY29y
ZHM7Ci0JUGVuZGluZ1dhbFN0YXRzLndhbF9mcGkgPSBkaWZmLndhbF9mcGk7Ci0JUGVuZGlu
Z1dhbFN0YXRzLndhbF9ieXRlcyA9IGRpZmYud2FsX2J5dGVzOwogCiAJaWYgKCFub3dhaXQp
CiAJCUxXTG9ja0FjcXVpcmUoJnN0YXRzX3NobWVtLT5sb2NrLCBMV19FWENMVVNJVkUpOwog
CWVsc2UgaWYgKCFMV0xvY2tDb25kaXRpb25hbEFjcXVpcmUoJnN0YXRzX3NobWVtLT5sb2Nr
LCBMV19FWENMVVNJVkUpKQogCQlyZXR1cm4gdHJ1ZTsKIAotI2RlZmluZSBXQUxTVEFUX0FD
QyhmbGQpIHN0YXRzX3NobWVtLT5zdGF0cy5mbGQgKz0gUGVuZGluZ1dhbFN0YXRzLmZsZAot
CVdBTFNUQVRfQUNDKHdhbF9yZWNvcmRzKTsKLQlXQUxTVEFUX0FDQyh3YWxfZnBpKTsKLQlX
QUxTVEFUX0FDQyh3YWxfYnl0ZXMpOwotCVdBTFNUQVRfQUNDKHdhbF9idWZmZXJzX2Z1bGwp
OwotCVdBTFNUQVRfQUNDKHdhbF93cml0ZSk7Ci0JV0FMU1RBVF9BQ0Mod2FsX3N5bmMpOwot
CVdBTFNUQVRfQUNDKHdhbF93cml0ZV90aW1lKTsKLQlXQUxTVEFUX0FDQyh3YWxfc3luY190
aW1lKTsKKyNkZWZpbmUgV0FMU1RBVF9BQ0MoZmxkLCB2YXJfdG9fYWRkKSBcCisgCShzdGF0
c19zaG1lbS0+c3RhdHMuZmxkICs9IHZhcl90b19hZGQuZmxkKQorI2RlZmluZSBXQUxMU1RB
VF9BQ0NfSU5TVFJfVElNRV9UWVBFKGZsZCkgXAorCShzdGF0c19zaG1lbS0+c3RhdHMuZmxk
ICs9IElOU1RSX1RJTUVfR0VUX01JQ1JPU0VDKFBlbmRpbmdXYWxTdGF0cy5mbGQpKQorCVdB
TFNUQVRfQUNDKHdhbF9yZWNvcmRzLCBkaWZmKTsKKwlXQUxTVEFUX0FDQyh3YWxfZnBpLCBk
aWZmKTsKKwlXQUxTVEFUX0FDQyh3YWxfYnl0ZXMsIGRpZmYpOworCVdBTFNUQVRfQUNDKHdh
bF9idWZmZXJzX2Z1bGwsIFBlbmRpbmdXYWxTdGF0cyk7CisJV0FMU1RBVF9BQ0Mod2FsX3dy
aXRlLCBQZW5kaW5nV2FsU3RhdHMpOworCVdBTFNUQVRfQUNDKHdhbF9zeW5jLCBQZW5kaW5n
V2FsU3RhdHMpOworCVdBTExTVEFUX0FDQ19JTlNUUl9USU1FX1RZUEUod2FsX3dyaXRlX3Rp
bWUpOworCVdBTExTVEFUX0FDQ19JTlNUUl9USU1FX1RZUEUod2FsX3N5bmNfdGltZSk7Cisj
dW5kZWYgV0FMTFNUQVRfQUNDX0lOU1RSX1RJTUVfVFlQRQogI3VuZGVmIFdBTFNUQVRfQUND
CiAKIAlMV0xvY2tSZWxlYXNlKCZzdGF0c19zaG1lbS0+bG9jayk7CmRpZmYgLS1naXQgYS9z
cmMvaW5jbHVkZS9wZ3N0YXQuaCBiL3NyYy9pbmNsdWRlL3Bnc3RhdC5oCmluZGV4IGRiOTY3
NTg4NGYzLi5jYzEwM2RmN2QyZSAxMDA2NDQKLS0tIGEvc3JjL2luY2x1ZGUvcGdzdGF0LmgK
KysrIGIvc3JjL2luY2x1ZGUvcGdzdGF0LmgKQEAgLTQ0NSw2ICs0NDUsMjIgQEAgdHlwZWRl
ZiBzdHJ1Y3QgUGdTdGF0X1dhbFN0YXRzCiAJVGltZXN0YW1wVHogc3RhdF9yZXNldF90aW1l
c3RhbXA7CiB9IFBnU3RhdF9XYWxTdGF0czsKIAorLyoKKyAqIENyZWF0ZWQgZm9yIGFjY3Vt
dWxhdGluZyB3YWxfd3JpdGVfdGltZSBhbmQgd2FsX3N5bmNfdGltZSBhcyBhIGluc3RyX3Rp
bWUKKyAqIGJ1dCBpbnN0cl90aW1lIGNhbid0IGJlIHVzZWQgYXMgYSB0eXBlIHdoZXJlIGl0
IGVuZHMgdXAgb24tZGlzaworICogYmVjYXVzZSBpdHMgdW5pdHMgbWF5IGNoYW5nZS4gUGdT
dGF0X1dhbFN0YXRzIHR5cGUgaXMgdXNlZCBmb3IKKyAqIGluLW1lbW9yeS9vbi1kaXNrIGRh
dGEuIFNvLCBQZ1N0YXRfUGVuZGluZ1dhbFVzYWdlIGlzIGNyZWF0ZWQgZm9yCisgKiBhY2N1
bXVsYXRpbmcgaW50ZXJ2YWxzIGFzIGEgaW5zdHJfdGltZS4KKyAqLwordHlwZWRlZiBzdHJ1
Y3QgUGdTdGF0X1BlbmRpbmdXYWxVc2FnZQoreworCVBnU3RhdF9Db3VudGVyIHdhbF9idWZm
ZXJzX2Z1bGw7CisJUGdTdGF0X0NvdW50ZXIgd2FsX3dyaXRlOworCVBnU3RhdF9Db3VudGVy
IHdhbF9zeW5jOworCWluc3RyX3RpbWUgd2FsX3dyaXRlX3RpbWU7CisJaW5zdHJfdGltZSB3
YWxfc3luY190aW1lOworfSBQZ1N0YXRfUGVuZGluZ1dhbFVzYWdlOworCiAKIC8qCiAgKiBG
dW5jdGlvbnMgaW4gcGdzdGF0LmMKQEAgLTc1OCw3ICs3NzQsNyBAQCBleHRlcm4gUEdETExJ
TVBPUlQgU2Vzc2lvbkVuZFR5cGUgcGdTdGF0U2Vzc2lvbkVuZENhdXNlOwogICovCiAKIC8q
IHVwZGF0ZWQgZGlyZWN0bHkgYnkgYmFja2VuZHMgYW5kIGJhY2tncm91bmQgcHJvY2Vzc2Vz
ICovCi1leHRlcm4gUEdETExJTVBPUlQgUGdTdGF0X1dhbFN0YXRzIFBlbmRpbmdXYWxTdGF0
czsKK2V4dGVybiBQR0RMTElNUE9SVCBQZ1N0YXRfUGVuZGluZ1dhbFVzYWdlIFBlbmRpbmdX
YWxTdGF0czsKIAogCiAjZW5kaWYJCQkJCQkJLyogUEdTVEFUX0ggKi8KLS0gCjIuMzkuMQoK


--------------bznyIeA1GF2EfYLd5FwvujdK--