Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lycDg-00075b-9z for pgsql-hackers@arkaria.postgresql.org; Wed, 30 Jun 2021 15:33:44 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1lycDe-0003cA-16 for pgsql-hackers@arkaria.postgresql.org; Wed, 30 Jun 2021 15:33:42 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lycDd-0003c2-LZ for pgsql-hackers@lists.postgresql.org; Wed, 30 Jun 2021 15:33:41 +0000 Received: from sfr-1.cri.mines-paristech.fr ([77.158.180.225] helo=smtp.cri.ensmp.fr) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lycDZ-0001QL-Iu for pgsql-hackers@postgresql.org; Wed, 30 Jun 2021 15:33:40 +0000 Received: from pseudo (nat-2.mines-paristech.fr [77.158.173.9]) by smtp.cri.ensmp.fr (Postfix) with ESMTPSA id BAEF9C019D; Wed, 30 Jun 2021 17:35:32 +0200 (CEST) Date: Wed, 30 Jun 2021 17:33:24 +0200 (CEST) From: Fabien COELHO X-X-Sender: fabien@pseudo To: Yugo NAGATA cc: Thomas Munro , Marina Polyakova , Alvaro Herrera , pgsql-hackers , Teodor Sigaev Subject: Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors In-Reply-To: <20210701000213.bd91004653d86255979bbdc0@sraoss.co.jp> Message-ID: References: <20181116195952.nq4ak4r7cbzvoher@alvherre.pgsql> <20210524112910.444fbfdfbff747bd3b9720ee@sraoss.co.jp> <20210622235459.4b2e38e268488014226b3262@sraoss.co.jp> <20210701000213.bd91004653d86255979bbdc0@sraoss.co.jp> User-Agent: Alpine 2.22 (DEB 394 2020-01-19) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="2056645051-1317854402-1625067214=:2013120" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --2056645051-1317854402-1625067214=:2013120 Content-Type: text/plain; charset=ISO-2022-JP; format=flowed Hello Yugo-san, Thanks for the update! >> Patch seems to apply cleanly with "git apply", but does not compile on my >> host: "undefined reference to `conditional_stack_reset'". >> >> However it works better when using the "patch". I'm wondering why git >> apply fails silently… > > Hmm, I don't know why your compiling fails... I can apply and complile > successfully using git. Hmmm. Strange! >> Given that we manage errors, ISTM that we should not necessarily stop >> on other not retried errors, but rather count/report them and possibly >> proceed. Eg with something like: [...] We could count the fail, >> rollback if necessary, and go on. What do you think? Maybe such >> behavior would deserve an option. > > This feature to count failures that could occur at runtime seems nice. However, > as discussed in [1], I think it is better to focus to only failures that can be > retried in this patch, and introduce the feature to handle other failures in a > separate patch. Ok. >> --report-latencies -> --report-per-command: should we keep supporting >> the previous option? > > Ok. Although now the option is not only for latencies, considering users who > are using the existing option, I'm fine with this. I got back this to the > previous name. Hmmm. I liked the new name! My point was whether we need to support the old one as well for compatibility, or whether we should not bother. I'm still wondering. As I think that the new name is better, I'd suggest to keep it. >> --failures-detailed: if we bother to run with handling failures, should >> it always be on? > > If we print other failures that cannot be retried in future, it could a lot > of lines and might make some users who don't need details of failures annoyed. > Moreover, some users would always need information of detailed failures in log, > and others would need only total numbers of failures. Ok. > Currently we handle only serialization and deadlock failures, so the number of > lines printed and the number of columns of logging is not large even under the > failures-detail, but if we have a chance to handle other failures in future, > ISTM adding this option makes sense considering users who would like simple > outputs. Hmmm. What kind of failures could be managed with retries? I guess that on a connection failure we can try to reconnect, but otherwise it is less clear that other failures make sense to retry. >> --debug-errors: I'm not sure we should want a special debug mode for that, >> I'd consider integrating it with the standard debug, or just for development. > > I think --debug is a debug option for telling users the pgbench's internal > behaviors, that is, which client is doing what. On other hand, --debug-errors > is for telling users what error caused a retry or a failure in detail. For > users who are not interested in pgbench's internal behavior (sending a command, > receiving a result, ... ) but interested in actual errors raised during running > script, this option seems useful. Ok. The this is not really about debug per se, but a verbosity setting? Maybe --verbose-errors would make more sense? I'm unsure. I'll think about it. >> Also, should it use pg_log_debug? > > If we use pg_log_debug, the message is printed only under --debug. > Therefore, I fixed to use pg_log_info instead of pg_log_error or fprintf. Ok, pg_log_info seems right. >> Tries vs retries: I'm at odds with having tries & retries and + 1 here >> and there to handle that, which is a little bit confusing. I'm wondering whether >> we could only count "tries" and adjust to report what we want later? > > I fixed to use "tries" instead of "retries" in CState. However, we still use > "retries" in StatsData and Command because the number of retries is printed > in the final result. Is it less confusing than the previous? I'm going to think about it. >> advanceConnectionState: ISTM that ERROR should logically be before others which >> lead to it. > > Sorry, I couldn't understand your suggestion. Is this about the order of case > statements or pg_log_error? My sentence got mixed up. My point was about the case order, so that they are put in a more logical order when reading all the cases. >> Currently, ISTM that the retry on error mode is implicitely always on. >> Do we want that? I'd say yes, but maybe people could disagree. > > The default values of max-tries is 1, so the retry on error is off. > Failed transactions are retried only when the user wants it and > specifies a valid value to max-treis. Ok. My point is that we do not stop on such errors, whereas before ISTM that we would have stopped, so somehow the default behavior has changed and the previous behavior cannot be reinstated with an option. Maybe that is not bad, but this is a behavioral change which needs to be documented and argumented. >> See the attached files for generating deadlocks reliably (start with 2 >> clients). What do you think? The PL/pgSQL minimal, it is really >> client-code oriented. > > Sorry, but I cannot find the attached file. Sorry. Attached to this mail. The serialization stuff does not seem to work as well as the deadlock one. Run with 2 clients. -- Fabien. --2056645051-1317854402-1625067214=:2013120 Content-Type: application/x-sql; name=deadlock_prep.sql Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename=deadlock_prep.sql LS0gY2xlYW51cApEUk9QIFNFUVVFTkNFIElGIEVYSVNUUyBwZ2JlbmNoX2Rl YWRsb2NrX3NlcTsKRFJPUCBUQUJMRSBJRiBFWElTVFMgcGdiZW5jaF9kZWFk bG9ja19kYXRhOwpEUk9QIFBST0NFRFVSRSBJRiBFWElTVFMgcGdiZW5jaF9k ZWFkbG9ja193YWl0OwotLSBob3AKQ1JFQVRFIFNFUVVFTkNFIHBnYmVuY2hf ZGVhZGxvY2tfc2VxOwpDUkVBVEUgVEFCTEUgcGdiZW5jaF9kZWFkbG9ja19k YXRhKAogIGNpZCBJTlRFR0VSIFBSSU1BUlkgS0VZLAogIG15aWQgSU5URUdF UiBOT1QgTlVMTCBERUZBVUxUIDAKKTsKSU5TRVJUIElOVE8gcGdiZW5jaF9k ZWFkbG9ja19kYXRhKGNpZCkgVkFMVUVTICgxKSwgKDIpOwpDUkVBVEUgUFJP Q0VEVVJFIHBnYmVuY2hfZGVhZGxvY2tfd2FpdChpZCBJTlRFR0VSKSBBUyAk JApERUNMQVJFCiAgaSBJTlRFR0VSIE5PVCBOVUxMIERFRkFVTFQgMDsKQkVH SU4KICAtLSBhY3RpdmUgd2FpdCBsb29wLCB3aGljaCByZWxpZXMgb24gdGhl IGZhY3QgdGhhdCBhIHNlcSBpcyBvdXQgb2YgYSB0cmFuc2FjdGlvbgogIExP T1AKICAgIFNFTEVDVCBsYXN0X3ZhbHVlIElOVE8gaSBGUk9NIHBnYmVuY2hf ZGVhZGxvY2tfc2VxOwogICAgRVhJVCBXSEVOIGkgPj0gaWQ7CiAgRU5EIExP T1A7CkVORDsKJCQgTEFOR1VBR0UgcGxwZ3NxbDsKLS0gYWRkIHN0YXRlbWVu dCB0aW1lb3V0Pwo= --2056645051-1317854402-1625067214=:2013120 Content-Type: application/x-sql; name=deadlock.sql Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename=deadlock.sql LS0gZ2VuZXJhdGUgYSBkZWFkbG9jawotLSBkZWFkbG9jayBkZXRlY3Rpb24g cnVucyBhYm91dCBldmVyeSBzZWNvbmQsCi0tIGhlbmNlIHdlIHNob3VsZCBl eHBlY3QgMSB0cHMKQkVHSU47ClNFTEVDVCBORVhUVkFMKCdwZ2JlbmNoX2Rl YWRsb2NrX3NlcScpIEFTIG15aWQgXGdzZXQKXHNldCBvZGQgOm15aWQgJSAy ID0gMQpcaWYgOm9kZAotLSB3YWl0IGZvciBFVkVOIHRvIGdldCBpdHMgaWQg YXMgd2VsbApDQUxMIHBnYmVuY2hfZGVhZGxvY2tfd2FpdCg6bXlpZCsxKTsK VVBEQVRFIHBnYmVuY2hfZGVhZGxvY2tfZGF0YSBTRVQgbXlpZCA9IDpteWlk IFdIRVJFIGNpZCA9IDE7Ci0tIGJhcnJpZXIKU0VMRUNUIE5FWFRWQUwoJ3Bn YmVuY2hfZGVhZGxvY2tfc2VxJyk7CkNBTEwgcGdiZW5jaF9kZWFkbG9ja193 YWl0KDpteWlkKzMpOwpVUERBVEUgcGdiZW5jaF9kZWFkbG9ja19kYXRhIFNF VCBteWlkID0gOm15aWQgV0hFUkUgY2lkID0gMjsKXGVsc2UKLS0gRVZFTgpV UERBVEUgcGdiZW5jaF9kZWFkbG9ja19kYXRhIFNFVCBteWlkID0gOm15aWQg V0hFUkUgY2lkID0gMjsKLS0gYmFycmllcgpTRUxFQ1QgTkVYVFZBTCgncGdi ZW5jaF9kZWFkbG9ja19zZXEnKTsKQ0FMTCBwZ2JlbmNoX2RlYWRsb2NrX3dh aXQoOm15aWQrMik7ClVQREFURSBwZ2JlbmNoX2RlYWRsb2NrX2RhdGEgU0VU IG15aWQgPSA6bXlpZCBXSEVSRSBjaWQgPSAxOwpcZW5kaWYKLS0gUkVTRVQg YnkgdGhlIGRlYWRsb2NrIHdpbm5lcgpcc2V0IGkgZGVidWcoOm15aWQpClVQ REFURSBwZ2JlbmNoX2RlYWRsb2NrX2RhdGEgU0VUIG15aWQgPSAwOwpDT01N SVQ7Cg== --2056645051-1317854402-1625067214=:2013120 Content-Type: application/x-sql; name=serializable.sql Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename=serializable.sql LS0gZ2VuZXJhdGUgYSBzZXJpYWxpemF0aW9uIGVycm9yLgpCRUdJTiBJU09M QVRJT04gTEVWRUwgU0VSSUFMSVpBQkxFOwpTRUxFQ1QgTkVYVFZBTCgncGdi ZW5jaF9kZWFkbG9ja19zZXEnKSBBUyBteWlkIFxnc2V0ClxzZXQgb2RkIDpt eWlkICUgMiA9IDEKXGlmIDpvZGQKLS0gd2FpdCBmb3IgRVZFTiB0byBnZXQg aXRzIGlkIGFzIHdlbGwKQ0FMTCBwZ2JlbmNoX2RlYWRsb2NrX3dhaXQoOm15 aWQrMSk7ClNFTEVDVCAqIEZST00gcGdiZW5jaF9kZWFkbG9ja19kYXRhIFdI RVJFIGNpZCA9IDE7Ci0tIGJhcnJpZXIKU0VMRUNUIE5FWFRWQUwoJ3BnYmVu Y2hfZGVhZGxvY2tfc2VxJyk7CkNBTEwgcGdiZW5jaF9kZWFkbG9ja193YWl0 KDpteWlkKzMpOwpVUERBVEUgcGdiZW5jaF9kZWFkbG9ja19kYXRhIFNFVCBt eWlkID0gOm15aWQgV0hFUkUgY2lkID0gMjsKXGVsc2UKLS0gRVZFTgpTRUxF Q1QgKiBGUk9NIHBnYmVuY2hfZGVhZGxvY2tfZGF0YSBXSEVSRSBjaWQgPSAy OwotLSBiYXJyaWVyClNFTEVDVCBORVhUVkFMKCdwZ2JlbmNoX2RlYWRsb2Nr X3NlcScpOwpDQUxMIHBnYmVuY2hfZGVhZGxvY2tfd2FpdCg6bXlpZCsyKTsK VVBEQVRFIHBnYmVuY2hfZGVhZGxvY2tfZGF0YSBTRVQgbXlpZCA9IDpteWlk IFdIRVJFIGNpZCA9IDE7ClxlbmRpZgotLSBSRVNFVCAKXHNldCBpIGRlYnVn KDpteWlkKQpDT01NSVQ7Cg== --2056645051-1317854402-1625067214=:2013120--