Content-Type: multipart/mixed; boundary="------------mFys4kACal8MMhXmzLxtN8q1"
Message-ID: <85fe426d-248a-47f7-8784-22af811ee572@oss.nttdata.com>
Date: Fri, 8 Nov 2024 00:54:58 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Fix for Extra Parenthesis in pgbench progress message
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: btogiwarayuushi@oss.nttdata.com, pgsql-hackers@postgresql.org
References: <9a9b8b95b6a709877ae48ad5b0c59bb9@oss.nttdata.com>
 <20241102.204310.303633563778001008.ishii@postgresql.org>
 <0f7ac0fa-f26b-49dc-b9a0-d35c29ace3a5@oss.nttdata.com>
 <20241107.104143.1498084100859794666.ishii@postgresql.org>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <20241107.104143.1498084100859794666.ishii@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/85fe426d-248a-47f7-8784-22af811ee572%40oss.nttdata.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------mFys4kACal8MMhXmzLxtN8q1
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024/11/07 10:41, Tatsuo Ishii wrote:
>>> The patch works perfectly for the case that there is one extra brace
>>> as shown in your example. However I think it will not work if there
>>> are two or more extra braces.
>>
>> Are you suggesting adding more space characters before the carriage
>> return
>> in the progress reporting line, like this?
> 
> No. I thought about dynamically calculating spaces needed to be
> printed something like attached patch.
> 
>> Since the line includes
>> both
>> elapsed and remaining times, its total length doesn’t change much.
> 
> Maybe. But I am also worried about the case when we would want to
> change the log line format in the future. We might introduce this kind
> of bug again.  By dynamically calculating the number of necessary
> spaces, we don't need to think about the concern.

+1

+			if (previous_chars != 0)
+			{
+				int	n_spaces = chars - previous_chars;
+				fprintf(stderr, "%*s", n_spaces, "");
+				chars += n_spaces;
+			}

Currently, this is added only when use_quiet is false, but shouldn’t it also apply when use_quiet is true?

Also, what happens if chars is smaller than previous_chars?

I’m unsure why chars needs to be incremented by n_spaces.


I’ve created an updated patch based on your idea—could you take a look?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

--------------mFys4kACal8MMhXmzLxtN8q1
Content-Type: text/plain; charset=UTF-8;
 name="v3-0001-pgbench-Ensure-previous-progress-message-is-fully.patch"
Content-Disposition: attachment;
 filename*0="v3-0001-pgbench-Ensure-previous-progress-message-is-fully.pa";
 filename*1="tch"
Content-Transfer-Encoding: base64

RnJvbSA3ZjBkMTBlOWMyYzY1NTI3ZmJiMDFhY2Q3NzczNjZkMDA1OTZlNzJjIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBNYXNhbyA8ZnVqaWlAcG9zdGdyZXNxbC5v
cmc+CkRhdGU6IEZyaSwgOCBOb3YgMjAyNCAwMDozNDo1NSArMDkwMApTdWJqZWN0OiBbUEFU
Q0ggdjNdIHBnYmVuY2g6IEVuc3VyZSBwcmV2aW91cyBwcm9ncmVzcyBtZXNzYWdlIGlzIGZ1
bGx5IGNsZWFyZWQKIHdoZW4gdXBkYXRpbmcuCgpEdXJpbmcgcGdiZW5jaCdzIHRhYmxlIGlu
aXRpYWxpemF0aW9uLCBwcm9ncmVzcyB1cGRhdGVzIGNvdWxkIGRpc3BsYXkKbGVmdG92ZXIg
Y2hhcmFjdGVycyBmcm9tIHRoZSBwcmV2aW91cyBtZXNzYWdlIGlmIHRoZSBuZXcgbWVzc2Fn
ZQp3YXMgc2hvcnRlci4gVGhpcyBjb21taXQgcmVzb2x2ZXMgdGhlIGlzc3VlIGJ5IGFwcGVu
ZGluZyBzcGFjZXMgdG8KdGhlIGN1cnJlbnQgbWVzc2FnZSB0byBmdWxseSBvdmVyd3JpdGUg
YW55IHJlbWFpbmluZyBjaGFyYWN0ZXJzIGZyb20KdGhlIHByZXZpb3VzIGxpbmUuCgpCYWNr
LXBhdGNoIHRvIGFsbCB0aGUgc3VwcG9ydGVkIHZlcnNpb25zLgoKQXV0aG9yOiBZdXNoaSBP
Z2l3YXJhLCBUYXRzdW8gSXNoaWksIEZ1amlpIE1hc2FvClJldmlld2VkLWJ5OiBUYXRzdW8g
SXNoaWksIEZ1amlpIE1hc2FvCkRpc2N1c3Npb246IGh0dHBzOi8vcG9zdGdyLmVzL20vOWE5
YjhiOTViNmE3MDk4NzdhZTQ4YWQ1YjBjNTliYjlAb3NzLm50dGRhdGEuY29tCi0tLQogc3Jj
L2Jpbi9wZ2JlbmNoL3BnYmVuY2guYyB8IDE5ICsrKysrKysrKysrKysrKy0tLS0KIDEgZmls
ZSBjaGFuZ2VkLCAxNSBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdp
dCBhL3NyYy9iaW4vcGdiZW5jaC9wZ2JlbmNoLmMgYi9zcmMvYmluL3BnYmVuY2gvcGdiZW5j
aC5jCmluZGV4IGJhMjQ3ZjNmODUuLjE5YTgyMjQ0YjUgMTAwNjQ0Ci0tLSBhL3NyYy9iaW4v
cGdiZW5jaC9wZ2JlbmNoLmMKKysrIGIvc3JjL2Jpbi9wZ2JlbmNoL3BnYmVuY2guYwpAQCAt
NDk0NCw2ICs0OTQ0LDcgQEAgaW5pdFBvcHVsYXRlVGFibGUoUEdjb25uICpjb24sIGNvbnN0
IGNoYXIgKnRhYmxlLCBpbnQ2NCBiYXNlLAogCWludAkJCW47CiAJaW50NjQJCWs7CiAJaW50
CQkJY2hhcnMgPSAwOworCWludAkJCXByZXZfY2hhcnMgPSAwOwogCVBHcmVzdWx0ICAgKnJl
czsKIAlQUUV4cEJ1ZmZlckRhdGEgc3FsOwogCWNoYXIJCWNvcHlfc3RhdGVtZW50WzI1Nl07
CkBAIC01MDA0LDEwICs1MDA1LDEwIEBAIGluaXRQb3B1bGF0ZVRhYmxlKFBHY29ubiAqY29u
LCBjb25zdCBjaGFyICp0YWJsZSwgaW50NjQgYmFzZSwKIAkJCWRvdWJsZQkJZWxhcHNlZF9z
ZWMgPSBQR19USU1FX0dFVF9ET1VCTEUocGdfdGltZV9ub3coKSAtIHN0YXJ0KTsKIAkJCWRv
dWJsZQkJcmVtYWluaW5nX3NlYyA9ICgoZG91YmxlKSB0b3RhbCAtIGopICogZWxhcHNlZF9z
ZWMgLyBqOwogCi0JCQljaGFycyA9IGZwcmludGYoc3RkZXJyLCBJTlQ2NF9GT1JNQVQgIiBv
ZiAiIElOVDY0X0ZPUk1BVCAiIHR1cGxlcyAoJWQlJSkgb2YgJXMgZG9uZSAoZWxhcHNlZCAl
LjJmIHMsIHJlbWFpbmluZyAlLjJmIHMpJWMiLAorCQkJY2hhcnMgPSBmcHJpbnRmKHN0ZGVy
ciwgSU5UNjRfRk9STUFUICIgb2YgIiBJTlQ2NF9GT1JNQVQgIiB0dXBsZXMgKCVkJSUpIG9m
ICVzIGRvbmUgKGVsYXBzZWQgJS4yZiBzLCByZW1haW5pbmcgJS4yZiBzKSIsCiAJCQkJCQkJ
aiwgdG90YWwsCiAJCQkJCQkJKGludCkgKChqICogMTAwKSAvIHRvdGFsKSwKLQkJCQkJCQl0
YWJsZSwgZWxhcHNlZF9zZWMsIHJlbWFpbmluZ19zZWMsIGVvbCk7CisJCQkJCQkJdGFibGUs
IGVsYXBzZWRfc2VjLCByZW1haW5pbmdfc2VjKTsKIAkJfQogCQkvKiBsZXQncyBub3QgY2Fs
bCB0aGUgdGltaW5nIGZvciBlYWNoIHJvdywgYnV0IG9ubHkgZWFjaCAxMDAgcm93cyAqLwog
CQllbHNlIGlmICh1c2VfcXVpZXQgJiYgKGogJSAxMDAgPT0gMCkpCkBAIC01MDE4LDE1ICs1
MDE5LDI1IEBAIGluaXRQb3B1bGF0ZVRhYmxlKFBHY29ubiAqY29uLCBjb25zdCBjaGFyICp0
YWJsZSwgaW50NjQgYmFzZSwKIAkJCS8qIGhhdmUgd2UgcmVhY2hlZCB0aGUgbmV4dCBpbnRl
cnZhbCAob3IgZW5kKT8gKi8KIAkJCWlmICgoaiA9PSB0b3RhbCkgfHwgKGVsYXBzZWRfc2Vj
ID49IGxvZ19pbnRlcnZhbCAqIExPR19TVEVQX1NFQ09ORFMpKQogCQkJewotCQkJCWNoYXJz
ID0gZnByaW50ZihzdGRlcnIsIElOVDY0X0ZPUk1BVCAiIG9mICIgSU5UNjRfRk9STUFUICIg
dHVwbGVzICglZCUlKSBvZiAlcyBkb25lIChlbGFwc2VkICUuMmYgcywgcmVtYWluaW5nICUu
MmYgcyklYyIsCisJCQkJY2hhcnMgPSBmcHJpbnRmKHN0ZGVyciwgSU5UNjRfRk9STUFUICIg
b2YgIiBJTlQ2NF9GT1JNQVQgIiB0dXBsZXMgKCVkJSUpIG9mICVzIGRvbmUgKGVsYXBzZWQg
JS4yZiBzLCByZW1haW5pbmcgJS4yZiBzKSIsCiAJCQkJCQkJCWosIHRvdGFsLAogCQkJCQkJ
CQkoaW50KSAoKGogKiAxMDApIC8gdG90YWwpLAotCQkJCQkJCQl0YWJsZSwgZWxhcHNlZF9z
ZWMsIHJlbWFpbmluZ19zZWMsIGVvbCk7CisJCQkJCQkJCXRhYmxlLCBlbGFwc2VkX3NlYywg
cmVtYWluaW5nX3NlYyk7CiAKIAkJCQkvKiBza2lwIHRvIHRoZSBuZXh0IGludGVydmFsICov
CiAJCQkJbG9nX2ludGVydmFsID0gKGludCkgY2VpbChlbGFwc2VkX3NlYyAvIExPR19TVEVQ
X1NFQ09ORFMpOwogCQkJfQogCQl9CisKKwkJLyoKKwkJICogSWYgdGhlIHByZXZpb3VzIHBy
b2dyZXNzIG1lc3NhZ2UgaXMgbG9uZ2VyIHRoYW4gdGhlIGN1cnJlbnQgb25lLAorCQkgKiBh
ZGQgc3BhY2VzIHRvIHRoZSBjdXJyZW50IGxpbmUgdG8gZnVsbHkgb3ZlcndyaXRlIGFueSBy
ZW1haW5pbmcKKwkJICogY2hhcmFjdGVycyBmcm9tIHRoZSBwcmV2aW91cyBtZXNzYWdlLgor
CQkgKi8KKwkJaWYgKHByZXZfY2hhcnMgPiBjaGFycykKKwkJCWZwcmludGYoc3RkZXJyLCAi
JSpjIiwgcHJldl9jaGFycyAtIGNoYXJzLCAnICcpOworCQlmcHV0Yyhlb2wsIHN0ZGVycik7
CisJCXByZXZfY2hhcnMgPSBjaGFyczsKIAl9CiAKIAlpZiAoY2hhcnMgIT0gMCAmJiBlb2wg
IT0gJ1xuJykKLS0gCjIuNDcuMAoK

--------------mFys4kACal8MMhXmzLxtN8q1--