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 1oCG26-0005Oq-9J for pgsql-hackers@arkaria.postgresql.org; Fri, 15 Jul 2022 07:46:42 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oCG25-0000Fc-6L for pgsql-hackers@arkaria.postgresql.org; Fri, 15 Jul 2022 07:46:41 +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 1oCG24-00007m-Od for pgsql-hackers@lists.postgresql.org; Fri, 15 Jul 2022 07:46:40 +0000 Received: from oss.nttdata.com ([49.212.34.109]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oCG21-0001bI-7e for pgsql-hackers@lists.postgresql.org; Fri, 15 Jul 2022 07:46:39 +0000 Received: from [10.6.216.52] (unknown [203.180.48.65]) by oss.nttdata.com (Postfix) with ESMTPSA id BC9E860830; Fri, 15 Jul 2022 16:46:32 +0900 (JST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.6 at oss.nttdata.com Content-Type: multipart/mixed; boundary="------------kwt4Yiyo9CE3OI0R4zuFrEFk" Message-ID: <02347f81-304f-3a14-1a33-5f40dfd7da48@oss.nttdata.com> Date: Fri, 15 Jul 2022 16:46:32 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: Backup command and functions can cause assertion failure and segmentation fault Content-Language: en-US To: Michael Paquier , Robert Haas Cc: Masahiko Sawada , PostgreSQL Hackers References: <3374718f-9fbf-a950-6d66-d973e027f44c@oss.nttdata.com> <6a759108-a02c-34fd-856c-d7c09d29d2ba@oss.nttdata.com> From: Fujii Masao In-Reply-To: List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------kwt4Yiyo9CE3OI0R4zuFrEFk Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2022/07/14 17:00, Michael Paquier wrote: > On Fri, Jul 08, 2022 at 08:56:14AM -0400, Robert Haas wrote: >> On Thu, Jul 7, 2022 at 10:58 AM Fujii Masao wrote: >>> But if many think that it's worth adding the test, I will give a >>> try. But even in that case, I think it's better to commit the >>> proposed patch at first to fix the bug, and then to write the patch >>> adding the test. > > I have looked at that in details, Thanks! and it is possible to rely on > pg_stat_activity.wait_event to be BaseBackupThrottle, which would make ISTM that you can also use pg_stat_progress_basebackup.phase. > sure that the checkpoint triggered at the beginning of the backup > finishes and that we are in the middle of the base backup. The > command for the test should be a psql command with two -c switches > without ON_ERROR_STOP, so as the second pg_backup_stop() starts after > BASE_BACKUP is cancelled using the same connection, for something like > that: > psql -c "BASE_BACKUP (CHECKPOINT 'fast', MAX_RATE 32);" \ > -c "select pg_backup_stop()" "replication=database" > > The last part of the test should do a pump_until() and capture "backup > is not in progress" from the stderr output of the command run. > > This is leading me to the attached, that crashes quickly without the > fix and passes with the fix. Thanks for the patch! But I'm still not sure if it's worth adding only this test for the corner case while we don't have basic tests for BASE_BACKUP, pg_backup_start and pg_backup_stop. BTW, if we decide to add that test, are you planning to back-patch it? > >> It's true that we don't really have good test coverage of write-ahead >> logging and recovery, but this doesn't seem like the most important >> thing to be testing in that area, either, and developing stable tests >> for stuff like this can be a lot of work. > > Well, stability does not seem like a problem to me here. > >> I do kind of feel like the patch is fixing two separate bugs. The >> change to SendBaseBackup() is fixing the problem that, because there's >> SQL access on replication connections, we could try to start a backup >> in the middle of another backup by mixing and matching the two >> different methods of doing backups. The change to do_pg_abort_backup() >> is fixing the fact that, after aborting a base backup, we don't reset >> the session state properly so that another backup can be tried >> afterwards. >> >> I don't know if it's worth committing them separately - they are very >> small fixes. But it would probably at least be good to highlight in >> the commit message that there are two different issues. > > Grouping both fixes in the same commit sounds fine by me. No > objections from here. This sounds fine to me, too. On the other hand, it's also fine for me to push the changes separately so that we can easily identify each change later. So I separated the patch into two ones. Since one of them failed to be applied to v14 or before cleanly, I also created the patch for those back branches. So I attached three patches. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION --------------kwt4Yiyo9CE3OI0R4zuFrEFk Content-Type: text/plain; charset=UTF-8; name="0001-Prevent-BASE_BACKUP-in-the-middle-of-another-backup-_master_v15.patch" Content-Disposition: attachment; filename*0="0001-Prevent-BASE_BACKUP-in-the-middle-of-another-backup-_ma"; filename*1="ster_v15.patch" Content-Transfer-Encoding: base64 RnJvbSAxZDExMGJmMGZmM2JmYjUwODM3NDkzMGViODk0N2EyYTBkNWZmZTVlIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBNYXNhbyA8ZnVqaWlAcG9zdGdyZXNxbC5v cmc+CkRhdGU6IFR1ZSwgMTIgSnVsIDIwMjIgMDk6MzE6NTcgKzA5MDAKU3ViamVjdDogW1BB VENIXSBQcmV2ZW50IEJBU0VfQkFDS1VQIGluIHRoZSBtaWRkbGUgb2YgYW5vdGhlciBiYWNr dXAgaW4gdGhlCiBzYW1lIHNlc3Npb24uCgpNdWx0aXBsZSBub24tZXhjbHVzaXZlIGJhY2t1 cHMgYXJlIGFibGUgdG8gYmUgcnVuIGNvbnJyZW50bHkgaW4gZGlmZmVyZW50CnNlc3Npb25z LiBCdXQsIGluIHRoZSBzYW1lIHNlc3Npb24sIG9ubHkgb25lIG5vbi1leGNsdXNpdmUgYmFj a3VwIGNhbiBiZQpydW4gYXQgdGhlIHNhbWUgbW9tZW50LiBJZiBwZ19iYWNrdXBfc3RhcnQg KHBnX3N0YXJ0X2JhY2t1cCBpbiB2MTQgb3IgYmVmb3JlKQppcyBjYWxsZWQgaW4gdGhlIG1p ZGRsZSBvZiBhbm90aGVyIG5vbi1leGNsdXNpdmUgYmFja3VwIGluIHRoZSBzYW1lIHNlc3Np b24sCmFuIGVycm9yIGlzIHRocm93bi4KCkhvd2V2ZXIsIHByZXZpb3VzbHksIGluIGxvZ2lj YWwgcmVwbGljYXRpb24gd2Fsc2VuZGVyIG1vZGUsIGV2ZW4gaWYgdGhhdAp3YWxzZW5kZXIg c2Vzc2lvbiBoYWQgYWxyZWFkeSBjYWxsZWQgcGdfYmFja3VwX3N0YXJ0IGFuZCBzdGFydGVk CmEgbm9uLWV4Y2x1c2l2ZSBiYWNrdXAsIGl0IGNvdWxkIGV4ZWN1dGUgQkFTRV9CQUNLVVAg Y29tbWFuZCBhbmQKc3RhcnQgYW5vdGhlciBub24tZXhjbHVzaXZlIGJhY2t1cC4gV2hpY2gg Y2F1c2VkIHN1YnNlcXVlbnQgcGdfYmFja3VwX3N0b3AKdG8gdGhyb3cgYW4gZXJyb3IgYmVj YXVzZSBCQVNFX0JBQ0tVUCB1bmV4cGVjdGVkbHkgcmVzZXQgdGhlIHNlc3Npb24gc3RhdGUK bWFya2VkIGJ5IHBnX2JhY2t1cF9zdGFydC4KClRoaXMgY29tbWl0IHByZXZlbnRzIEJBU0Vf QkFDS1VQIGNvbW1hbmQgaW4gdGhlIG1pZGRsZSBvZiBhbm90aGVyCm5vbi1leGNsdXNpdmUg YmFja3VwIGluIHRoZSBzYW1lIHNlc3Npb24uCgpCYWNrLXBhdGNoIHRvIGFsbCBzdXBwb3J0 ZWQgYnJhbmNoZXMuCgpBdXRob3I6IEZ1amlpIE1hc2FvClJldmlld2VkLWJ5OiBLeW90YXJv IEhvcmlndWNoaSwgTWFzYWhpa28gU2F3YWRhLCBNaWNoYWVsIFBhcXVpZXIsIFJvYmVydCBI YWFzCkRpc2N1c3Npb246IGh0dHBzOi8vcG9zdGdyLmVzL20vMzM3NDcxOGYtOWZiZi1hOTUw LTZkNjYtZDk3M2UwMjdmNDRjQG9zcy5udHRkYXRhLmNvbQotLS0KIHNyYy9iYWNrZW5kL3Jl cGxpY2F0aW9uL2Jhc2ViYWNrdXAuYyB8IDYgKysrKysrCiAxIGZpbGUgY2hhbmdlZCwgNiBp bnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vYmFz ZWJhY2t1cC5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vYmFzZWJhY2t1cC5jCmluZGV4 IDk1NDQwMDEzYzAuLjYzN2MwY2U0NTkgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxp Y2F0aW9uL2Jhc2ViYWNrdXAuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9iYXNl YmFja3VwLmMKQEAgLTk0OSw2ICs5NDksMTIgQEAgU2VuZEJhc2VCYWNrdXAoQmFzZUJhY2t1 cENtZCAqY21kKQogewogCWJhc2ViYWNrdXBfb3B0aW9ucyBvcHQ7CiAJYmJzaW5rCSAgICpz aW5rOworCVNlc3Npb25CYWNrdXBTdGF0ZSBzdGF0dXMgPSBnZXRfYmFja3VwX3N0YXR1cygp OworCisJaWYgKHN0YXR1cyA9PSBTRVNTSU9OX0JBQ0tVUF9SVU5OSU5HKQorCQllcmVwb3J0 KEVSUk9SLAorCQkJCShlcnJjb2RlKEVSUkNPREVfT0JKRUNUX05PVF9JTl9QUkVSRVFVSVNJ VEVfU1RBVEUpLAorCQkJCSBlcnJtc2coImEgYmFja3VwIGlzIGFscmVhZHkgaW4gcHJvZ3Jl c3MgaW4gdGhpcyBzZXNzaW9uIikpKTsKIAogCXBhcnNlX2Jhc2ViYWNrdXBfb3B0aW9ucyhj bWQtPm9wdGlvbnMsICZvcHQpOwogCi0tIAoyLjM2LjAKCg== --------------kwt4Yiyo9CE3OI0R4zuFrEFk Content-Type: text/plain; charset=UTF-8; name="0001-Prevent-BASE_BACKUP-in-the-middle-of-another-backup-_v14_v10.patch" Content-Disposition: attachment; filename*0="0001-Prevent-BASE_BACKUP-in-the-middle-of-another-backup-_v1"; filename*1="4_v10.patch" Content-Transfer-Encoding: base64 RnJvbSAxMTZiNGViZTgxM2VkZWU2NjBiNjMzMDkwMDRmZjhmZmUxNGM1MzNjIE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBNYXNhbyA8ZnVqaWlAcG9zdGdyZXNxbC5v cmc+CkRhdGU6IFR1ZSwgMTIgSnVsIDIwMjIgMDk6MzE6NTcgKzA5MDAKU3ViamVjdDogW1BB VENIXSBQcmV2ZW50IEJBU0VfQkFDS1VQIGluIHRoZSBtaWRkbGUgb2YgYW5vdGhlciBiYWNr dXAgaW4gdGhlCiBzYW1lIHNlc3Npb24uCgpNdWx0aXBsZSBub24tZXhjbHVzaXZlIGJhY2t1 cHMgYXJlIGFibGUgdG8gYmUgcnVuIGNvbnJyZW50bHkgaW4gZGlmZmVyZW50CnNlc3Npb25z LiBCdXQsIGluIHRoZSBzYW1lIHNlc3Npb24sIG9ubHkgb25lIG5vbi1leGNsdXNpdmUgYmFj a3VwIGNhbiBiZQpydW4gYXQgdGhlIHNhbWUgbW9tZW50LiBJZiBwZ19iYWNrdXBfc3RhcnQg KHBnX3N0YXJ0X2JhY2t1cCBpbiB2MTQgb3IgYmVmb3JlKQppcyBjYWxsZWQgaW4gdGhlIG1p ZGRsZSBvZiBhbm90aGVyIG5vbi1leGNsdXNpdmUgYmFja3VwIGluIHRoZSBzYW1lIHNlc3Np b24sCmFuIGVycm9yIGlzIHRocm93bi4KCkhvd2V2ZXIsIHByZXZpb3VzbHksIGluIGxvZ2lj YWwgcmVwbGljYXRpb24gd2Fsc2VuZGVyIG1vZGUsIGV2ZW4gaWYgdGhhdAp3YWxzZW5kZXIg c2Vzc2lvbiBoYWQgYWxyZWFkeSBjYWxsZWQgcGdfYmFja3VwX3N0YXJ0IGFuZCBzdGFydGVk CmEgbm9uLWV4Y2x1c2l2ZSBiYWNrdXAsIGl0IGNvdWxkIGV4ZWN1dGUgQkFTRV9CQUNLVVAg Y29tbWFuZCBhbmQKc3RhcnQgYW5vdGhlciBub24tZXhjbHVzaXZlIGJhY2t1cC4gV2hpY2gg Y2F1c2VkIHN1YnNlcXVlbnQgcGdfYmFja3VwX3N0b3AKdG8gdGhyb3cgYW4gZXJyb3IgYmVj YXVzZSBCQVNFX0JBQ0tVUCB1bmV4cGVjdGVkbHkgcmVzZXQgdGhlIHNlc3Npb24gc3RhdGUK bWFya2VkIGJ5IHBnX2JhY2t1cF9zdGFydC4KClRoaXMgY29tbWl0IHByZXZlbnRzIEJBU0Vf QkFDS1VQIGNvbW1hbmQgaW4gdGhlIG1pZGRsZSBvZiBhbm90aGVyCm5vbi1leGNsdXNpdmUg YmFja3VwIGluIHRoZSBzYW1lIHNlc3Npb24uCgpCYWNrLXBhdGNoIHRvIGFsbCBzdXBwb3J0 ZWQgYnJhbmNoZXMuCgpBdXRob3I6IEZ1amlpIE1hc2FvClJldmlld2VkLWJ5OiBLeW90YXJv IEhvcmlndWNoaSwgTWFzYWhpa28gU2F3YWRhLCBNaWNoYWVsIFBhcXVpZXIsIFJvYmVydCBI YWFzCkRpc2N1c3Npb246IGh0dHBzOi8vcG9zdGdyLmVzL20vMzM3NDcxOGYtOWZiZi1hOTUw LTZkNjYtZDk3M2UwMjdmNDRjQG9zcy5udHRkYXRhLmNvbQotLS0KIHNyYy9iYWNrZW5kL3Jl cGxpY2F0aW9uL2Jhc2ViYWNrdXAuYyB8IDYgKysrKysrCiAxIGZpbGUgY2hhbmdlZCwgNiBp bnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vYmFz ZWJhY2t1cC5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vYmFzZWJhY2t1cC5jCmluZGV4 IGUwOTEwOGQwZWMuLmQxNDJjYzIxMzEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxp Y2F0aW9uL2Jhc2ViYWNrdXAuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9iYXNl YmFja3VwLmMKQEAgLTkzMiw2ICs5MzIsMTIgQEAgdm9pZAogU2VuZEJhc2VCYWNrdXAoQmFz ZUJhY2t1cENtZCAqY21kKQogewogCWJhc2ViYWNrdXBfb3B0aW9ucyBvcHQ7CisJU2Vzc2lv bkJhY2t1cFN0YXRlIHN0YXR1cyA9IGdldF9iYWNrdXBfc3RhdHVzKCk7CisKKwlpZiAoc3Rh dHVzID09IFNFU1NJT05fQkFDS1VQX05PTl9FWENMVVNJVkUpCisJCWVyZXBvcnQoRVJST1Is CisJCQkJKGVycmNvZGUoRVJSQ09ERV9PQkpFQ1RfTk9UX0lOX1BSRVJFUVVJU0lURV9TVEFU RSksCisJCQkJIGVycm1zZygiYSBiYWNrdXAgaXMgYWxyZWFkeSBpbiBwcm9ncmVzcyBpbiB0 aGlzIHNlc3Npb24iKSkpOwogCiAJcGFyc2VfYmFzZWJhY2t1cF9vcHRpb25zKGNtZC0+b3B0 aW9ucywgJm9wdCk7CiAKLS0gCjIuMzYuMAoK --------------kwt4Yiyo9CE3OI0R4zuFrEFk Content-Type: text/plain; charset=UTF-8; name="0002-Fix-assertion-failure-and-segmentation-fault-in-back.patch" Content-Disposition: attachment; filename*0="0002-Fix-assertion-failure-and-segmentation-fault-in-back.pa"; filename*1="tch" Content-Transfer-Encoding: base64 RnJvbSBhNzk4OTFlNzk5OGY3ZTBlMzNhMjA5NDcyNjQ3YTdhMTQxZDYzMTM0IE1vbiBTZXAg MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBGdWppaSBNYXNhbyA8ZnVqaWlAcG9zdGdyZXNxbC5v cmc+CkRhdGU6IFR1ZSwgMTIgSnVsIDIwMjIgMTE6NTM6MjkgKzA5MDAKU3ViamVjdDogW1BB VENIXSBGaXggYXNzZXJ0aW9uIGZhaWx1cmUgYW5kIHNlZ21lbnRhdGlvbiBmYXVsdCBpbiBi YWNrdXAgY29kZS4KCldoZW4gYSBub24tZXhjbHVzaXZlIGJhY2t1cCBpcyBjYW5jZWxlZCwg ZG9fcGdfYWJvcnRfYmFja3VwKCkgaXMgY2FsbGVkCmFuZCByZXNldHMgc29tZSB2YXJpYWJs ZXMgc2V0IGJ5IHBnX2JhY2t1cF9zdGFydCAocGdfc3RhcnRfYmFja3VwIGluIHYxNApvciBi ZWZvcmUpLiBCdXQgcHJldmlvdXNseSBpdCBmb3Jnb3QgdG8gcmVzZXQgdGhlIHNlc3Npb24g c3RhdGUgaW5kaWNhdGluZwp3aGV0aGVyIGEgbm9uLWV4Y2x1c2l2ZSBiYWNrdXAgaXMgaW4g cHJvZ3Jlc3Mgb3Igbm90IGluIHRoaXMgc2Vzc2lvbi4KClRoaXMgaXNzdWUgY291bGQgY2F1 c2UgYW4gYXNzZXJ0aW9uIGZhaWx1cmUgd2hlbiB0aGUgc2Vzc2lvbiBydW5uaW5nCkJBU0Vf QkFDS1VQIGlzIHRlcm1pbmF0ZWQgYWZ0ZXIgaXQgZXhlY3V0ZWQgcGdfYmFja3VwX3N0YXJ0 IGFuZApwZ19iYWNrdXBfc3RvcCAocGdfc3RvcF9iYWNrdXAgaW4gdjE0IG9yIGJlZm9yZSku IEFsc28gaXQgY291bGQgY2F1c2UKYSBzZWdtZW50YXRpb24gZmF1bHQgd2hlbiBwZ19iYWNr dXBfc3RvcCBpcyBjYWxsZWQgYWZ0ZXIgQkFTRV9CQUNLVVAKaW4gdGhlIHNhbWUgc2Vzc2lv biBpcyBjYW5jZWxlZC4KClRoaXMgY29tbWl0IGZpeGVzIHRoZSBpc3N1ZSBieSBtYWtpbmcg ZG9fcGdfYWJvcnRfYmFja3VwIHJlc2V0CnRoYXQgc2Vzc2lvbiBzdGF0ZS4KCkJhY2stcGF0 Y2ggdG8gYWxsIHN1cHBvcnRlZCBicmFuY2hlcy4KCkF1dGhvcjogRnVqaWkgTWFzYW8KUmV2 aWV3ZWQtYnk6IEt5b3Rhcm8gSG9yaWd1Y2hpLCBNYXNhaGlrbyBTYXdhZGEsIE1pY2hhZWwg UGFxdWllciwgUm9iZXJ0IEhhYXMKRGlzY3Vzc2lvbjogaHR0cHM6Ly9wb3N0Z3IuZXMvbS8z Mzc0NzE4Zi05ZmJmLWE5NTAtNmQ2Ni1kOTczZTAyN2Y0NGNAb3NzLm50dGRhdGEuY29tCi0t LQogc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0veGxvZy5jIHwgMiArKwogMSBmaWxlIGNo YW5nZWQsIDIgaW5zZXJ0aW9ucygrKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vz cy90cmFuc2FtL3hsb2cuYyBiL3NyYy9iYWNrZW5kL2FjY2Vzcy90cmFuc2FtL3hsb2cuYwpp bmRleCBiODA5YTIxNTJjLi45ODU0YjUxYzZhIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9h Y2Nlc3MvdHJhbnNhbS94bG9nLmMKKysrIGIvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0v eGxvZy5jCkBAIC04NzkxLDYgKzg3OTEsOCBAQCBkb19wZ19hYm9ydF9iYWNrdXAoaW50IGNv ZGUsIERhdHVtIGFyZykKIAl7CiAJCVhMb2dDdGwtPkluc2VydC5mb3JjZVBhZ2VXcml0ZXMg PSBmYWxzZTsKIAl9CisKKwlzZXNzaW9uQmFja3VwU3RhdGUgPSBTRVNTSU9OX0JBQ0tVUF9O T05FOwogCVdBTEluc2VydExvY2tSZWxlYXNlKCk7CiAKIAlpZiAoZW1pdF93YXJuaW5nKQot LSAKMi4zNi4wCgo= --------------kwt4Yiyo9CE3OI0R4zuFrEFk--