Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ilXQo-0001Pk-FH for pgsql-hackers@arkaria.postgresql.org; Sun, 29 Dec 2019 12:12:27 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1ilXQl-0000vW-7B for pgsql-hackers@arkaria.postgresql.org; Sun, 29 Dec 2019 12:12:23 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ilXQk-0000vP-Qk for pgsql-hackers@lists.postgresql.org; Sun, 29 Dec 2019 12:12:22 +0000 Received: from cyclops.postgrespro.ru ([93.174.131.138] helo=mail.postgrespro.ru) by magus.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1ilXQg-0004KN-88 for pgsql-hackers@postgresql.org; Sun, 29 Dec 2019 12:12:20 +0000 Received: from localhost (localhost [127.0.0.1]) by mail.postgrespro.ru (Postfix) with ESMTP id C52C121C2824; Sun, 29 Dec 2019 15:12:16 +0300 (MSK) X-Virus-Scanned: Debian amavisd-new at postgrespro.ru X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=x tagged_above=-99 required=4 WHITELISTED tests=[] autolearn=unavailable Received: from mail.postgrespro.ru (cyclops.l.postgrespro.ru [192.168.27.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mail.postgrespro.ru (Postfix) with ESMTPSA id 88E8F21C27F8; Sun, 29 Dec 2019 15:12:16 +0300 (MSK) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mail; t=1577621536; bh=/bq3uN3xDCbZIPtgnmWE4v7ZcUc5rGQOlXmgbKao5hQ=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=RrS3qtGNHSXYGmY1CttDfkwt5LK4ZEeFZ2q+zl/NhVXPl/mMwktnWYMx7UWal/n8i g13vHxOD/tsyD2YTd9awhimHhwCkF1WI75gPDFvZqhe4zwaXe0FTkPzxp4M//QxyqR SOVLn7OZUi0Oq1r7X8VTpDjvkW4XjKs5Rbm2vhj8= MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=_dbebaff8668074c640bc6916df585dbe" Content-Transfer-Encoding: 7bit Date: Sun, 29 Dec 2019 15:12:16 +0300 From: Alexey Kondratov To: Kyotaro Horiguchi Cc: pgsql-hackers@postgresql.org, michael@paquier.xyz, simon@2ndquadrant.com Subject: Re: Physical replication slot advance is not persistent In-Reply-To: <682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru> References: <20191225.130337.442597881772691026.horikyota.ntt@gmail.com> <24b9a157-d95e-96c2-2fde-9cf73f05008c@postgrespro.ru> <20191226.173349.2159296357637287391.horikyota.ntt@gmail.com> <682b1960-cb89-8738-dbdf-8033662fb80f@postgrespro.ru> User-Agent: Roundcube Webmail/1.4.0 Message-ID: <175c2760666a78205e053207794c0f8f@postgrespro.ru> X-Sender: a.kondratov@postgrespro.ru List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --=_dbebaff8668074c640bc6916df585dbe Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII; format=flowed On 2019-12-26 16:35, Alexey Kondratov wrote: > > Another concern is that ReplicationSlotIsDirty is added with the only > one user. It also cannot be used by SaveSlotToPath due to the > simultaneous usage of both flags dirty and just_dirtied there. > > In that way, I hope that we should call ReplicationSlotSave > unconditionally in the pg_replication_slot_advance, so slot will be > saved or not automatically based on the slot->dirty flag. In the same > time, ReplicationSlotsComputeRequiredXmin and > ReplicationSlotsComputeRequiredLSN should be called by anyone, who > modifies xmin and LSN fields in the slot. Otherwise, currently we are > getting some leaky abstractions. > It seems that there was even a race in the order of actions inside pg_replication_slot_advance, it did following: - ReplicationSlotMarkDirty(); - ReplicationSlotsComputeRequiredXmin(false); - ReplicationSlotsComputeRequiredLSN(); - ReplicationSlotSave(); 1) Mark slot as dirty, which actually does nothing immediately, but setting dirty flag; 2) Do compute new global required LSN; 3) Flush slot state to disk. If someone will utilise old WAL and after that crash will happen between steps 2) and 3), then we start with old value of restart_lsn, but without required WAL. I do not know how to properly reproduce it without gdb and power off, so the chance is pretty low, but still it could be a case. Logical slots were not affected again, since there was a proper operations order (with comments) and slot flushing routines inside LogicalConfirmReceivedLocation. Thus, in the attached patch I have decided to do not perform slot flushing in the pg_replication_slot_advance at all and do it in the pg_physical_replication_slot_advance instead, as it is done in the LogicalConfirmReceivedLocation. Since this bugfix have not moved forward during the week, I will put it on the 01.2020 commitfest. Kyotaro, if you do not object I will add you as a reviewer, as you have already gave a lot of feedback, thank you for that! Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company --=_dbebaff8668074c640bc6916df585dbe Content-Transfer-Encoding: base64 Content-Type: text/x-diff; name=v3-0001-Make-physical-slot-advance-to-be-persistent.patch Content-Disposition: attachment; filename=v3-0001-Make-physical-slot-advance-to-be-persistent.patch; size=4146 RnJvbSBjOTQ0NmJmYzYwYWUxMjVhZGVhNDc2YmNjNWI3ZDUwZjRiYWZhMzhiIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBbGV4ZXkgS29uZHJhdG92IDxrb25kcmF0b3YuYWxla3NleUBn bWFpbC5jb20+CkRhdGU6IFN1biwgMjkgRGVjIDIwMTkgMTQ6Mzg6MTMgKzAzMDAKU3ViamVjdDog W1BBVENIIHYzXSBNYWtlIHBoeXNpY2FsIHNsb3QgYWR2YW5jZSB0byBiZSBwZXJzaXN0ZW50CgpT dGFydGluZyBmcm9tIHYxMSBwZ19yZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UgcmVzdWx0IHdhcyBu b3QgcGVyc2lzdGVudAphZnRlciByZXN0YXJ0LiBUaGlzIHBhdGNoIGZpeGVzIHRoYXQgYnVnIGJ5 IHByb3BlciBzbG90IGZsdXNoaW5nIGlmIGl0CndhcyBtb2RpZmllZC4KLS0tCiBzcmMvYmFja2Vu ZC9yZXBsaWNhdGlvbi9sb2dpY2FsL2xvZ2ljYWwuYyB8IDEwICsrKysrKy0tCiBzcmMvYmFja2Vu ZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYyAgICAgICB8IDMxICsrKysrKysrKysrKysrKy0tLS0t LS0tCiAyIGZpbGVzIGNoYW5nZWQsIDI5IGluc2VydGlvbnMoKyksIDEyIGRlbGV0aW9ucygtKQoK ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvbG9naWNhbC5jIGIv c3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9sb2dpY2FsLmMKaW5kZXggN2UwNjYxNTg2 NC4uYmRiODJkYTM5YiAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNh bC9sb2dpY2FsLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9sb2dpY2Fs LmMKQEAgLTEwNDksNyArMTA0OSwxMCBAQCBMb2dpY2FsQ29uZmlybVJlY2VpdmVkTG9jYXRpb24o WExvZ1JlY1B0ciBsc24pCiAKIAkJU3BpbkxvY2tSZWxlYXNlKCZNeVJlcGxpY2F0aW9uU2xvdC0+ bXV0ZXgpOwogCi0JCS8qIGZpcnN0IHdyaXRlIG5ldyB4bWluIHRvIGRpc2ssIHNvIHdlIGtub3cg d2hhdCdzIHVwIGFmdGVyIGEgY3Jhc2ggKi8KKwkJLyoKKwkJICogRmlyc3Qgd3JpdGUgbmV3IHht aW4gYW5kL29yIExTTiB0byBkaXNrLCBzbyB3ZSBrbm93IHdoYXQncyB1cAorCQkgKiBhZnRlciBh IGNyYXNoLgorCQkgKi8KIAkJaWYgKHVwZGF0ZWRfeG1pbiB8fCB1cGRhdGVkX3Jlc3RhcnQpCiAJ CXsKIAkJCVJlcGxpY2F0aW9uU2xvdE1hcmtEaXJ0eSgpOwpAQCAtMTA1Nyw2ICsxMDYwLDEwIEBA IExvZ2ljYWxDb25maXJtUmVjZWl2ZWRMb2NhdGlvbihYTG9nUmVjUHRyIGxzbikKIAkJCWVsb2co REVCVUcxLCAidXBkYXRlZCB4bWluOiAldSByZXN0YXJ0OiAldSIsIHVwZGF0ZWRfeG1pbiwgdXBk YXRlZF9yZXN0YXJ0KTsKIAkJfQogCisJCS8qIENvbXB1dGUgZ2xvYmFsIHJlcXVpcmVkIExTTiBp ZiByZXN0YXJ0X2xzbiB3YXMgY2hhbmdlZCAqLworCQlpZiAodXBkYXRlZF9yZXN0YXJ0KQorCQkJ UmVwbGljYXRpb25TbG90c0NvbXB1dGVSZXF1aXJlZExTTigpOworCiAJCS8qCiAJCSAqIE5vdyB0 aGUgbmV3IHhtaW4gaXMgc2FmZWx5IG9uIGRpc2ssIHdlIGNhbiBsZXQgdGhlIGdsb2JhbCB2YWx1 ZQogCQkgKiBhZHZhbmNlLiBXZSBkbyBub3QgdGFrZSBQcm9jQXJyYXlMb2NrIG9yIHNpbWlsYXIg c2luY2Ugd2Ugb25seQpAQCAtMTA3MCw3ICsxMDc3LDYgQEAgTG9naWNhbENvbmZpcm1SZWNlaXZl ZExvY2F0aW9uKFhMb2dSZWNQdHIgbHNuKQogCQkJU3BpbkxvY2tSZWxlYXNlKCZNeVJlcGxpY2F0 aW9uU2xvdC0+bXV0ZXgpOwogCiAJCQlSZXBsaWNhdGlvblNsb3RzQ29tcHV0ZVJlcXVpcmVkWG1p bihmYWxzZSk7Ci0JCQlSZXBsaWNhdGlvblNsb3RzQ29tcHV0ZVJlcXVpcmVkTFNOKCk7CiAJCX0K IAl9CiAJZWxzZQpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vc2xvdGZ1bmNz LmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYwppbmRleCBiYTA4YWQ0MDVm Li45ZDBlOTlhZmI4IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVu Y3MuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9zbG90ZnVuY3MuYwpAQCAtMzcwLDYg KzM3MCwxOSBAQCBwZ19waHlzaWNhbF9yZXBsaWNhdGlvbl9zbG90X2FkdmFuY2UoWExvZ1JlY1B0 ciBtb3ZldG8pCiAJCU15UmVwbGljYXRpb25TbG90LT5kYXRhLnJlc3RhcnRfbHNuID0gbW92ZXRv OwogCQlTcGluTG9ja1JlbGVhc2UoJk15UmVwbGljYXRpb25TbG90LT5tdXRleCk7CiAJCXJldGxz biA9IG1vdmV0bzsKKworCQkvKgorCQkgKiBEaXJ0eSB0aGUgc2xvdCBhcyB3ZSB1cGRhdGVkIGRh dGEgdGhhdCBpcyBtZWFudCB0byBiZSBwZXJzaXN0ZW50CisJCSAqIGJldHdlZW4gcmVzdGFydHMs IGZsdXNoIGl0IGFuZCByZS1jb21wdXRlIGdsb2JhbCByZXF1aXJlZCBMU04uCisJCSAqCisJCSAq IElmIHdlIGNoYW5nZSB0aGUgb3JkZXIgb2Ygb3BlcmF0aW9ucyBhbmQgZG8gbm90IGZsdXNoIHNs b3QgYmVmb3JlCisJCSAqIHJlcXVpcmVkIExTTiBjb21wdXRpbmcsIHRoZW4gdGhlcmUgd2lsbCBi ZSBhIHJhY2UuICBMZWFzdCByZWNlbnQgV0FMCisJCSAqIHNlZ21lbnRzIG1heSBiZSBhbHJlYWR5 IHV0aWxpemVkLCBzbyBpZiB3ZSBjcmFzaCBhbmQgc3RhcnQgYWdhaW4gd2l0aAorCQkgKiBvbGQg cmVzdGFydF9sc24gdGhlcmUgd2lsbCBiZSBubyBXQUwgdG8gcHJvY2VlZCBwcm9wZXJseS4KKwkJ ICovCisJCVJlcGxpY2F0aW9uU2xvdE1hcmtEaXJ0eSgpOworCQlSZXBsaWNhdGlvblNsb3RTYXZl KCk7CisJCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWlyZWRMU04oKTsKIAl9CiAKIAlyZXR1 cm4gcmV0bHNuOwpAQCAtNDYzLDYgKzQ3NiwxMCBAQCBwZ19sb2dpY2FsX3JlcGxpY2F0aW9uX3Ns b3RfYWR2YW5jZShYTG9nUmVjUHRyIG1vdmV0bykKIAogCQlpZiAoY3R4LT5yZWFkZXItPkVuZFJl Y1B0ciAhPSBJbnZhbGlkWExvZ1JlY1B0cikKIAkJeworCQkJLyoKKwkJCSAqIEl0IGFsc28gdGFr ZXMgY2FyZSBhYm91dCBjb21wdXRpbmcgYW5kIHVwZGF0aW5nIGdsb2JhbAorCQkJICogcmVxdWly ZWQgeG1pbiBhbmQgTFNOIGlmIG5lZWRlZC4KKwkJCSAqLwogCQkJTG9naWNhbENvbmZpcm1SZWNl aXZlZExvY2F0aW9uKG1vdmV0byk7CiAKIAkJCS8qCkBAIC01NjQsNyArNTgxLDEwIEBAIHBnX3Jl cGxpY2F0aW9uX3Nsb3RfYWR2YW5jZShQR19GVU5DVElPTl9BUkdTKQogCQkJCQkJKHVpbnQzMikg KG1vdmV0byA+PiAzMiksICh1aW50MzIpIG1vdmV0bywKIAkJCQkJCSh1aW50MzIpIChtaW5sc24g Pj4gMzIpLCAodWludDMyKSBtaW5sc24pKSk7CiAKLQkvKiBEbyB0aGUgYWN0dWFsIHNsb3QgdXBk YXRlLCBkZXBlbmRpbmcgb24gdGhlIHNsb3QgdHlwZSAqLworCS8qCisJICogRG8gdGhlIGFjdHVh bCBzbG90IHVwZGF0ZSwgZGVwZW5kaW5nIG9uIHRoZSBzbG90IHR5cGUuICBTbG90IHdpbGwgYmUK KwkgKiBtYXJrZWQgYXMgZGlydHkgYW5kIGZsdXNoZWQgYnkgcGdfKl9yZXBsaWNhdGlvbl9zbG90 X2FkdmFuY2UgaWYgbmVlZGVkLgorCSAqLwogCWlmIChPaWRJc1ZhbGlkKE15UmVwbGljYXRpb25T bG90LT5kYXRhLmRhdGFiYXNlKSkKIAkJZW5kbHNuID0gcGdfbG9naWNhbF9yZXBsaWNhdGlvbl9z bG90X2FkdmFuY2UobW92ZXRvKTsKIAllbHNlCkBAIC01NzMsMTUgKzU5Myw2IEBAIHBnX3JlcGxp Y2F0aW9uX3Nsb3RfYWR2YW5jZShQR19GVU5DVElPTl9BUkdTKQogCXZhbHVlc1swXSA9IE5hbWVH ZXREYXR1bSgmTXlSZXBsaWNhdGlvblNsb3QtPmRhdGEubmFtZSk7CiAJbnVsbHNbMF0gPSBmYWxz ZTsKIAotCS8qIFVwZGF0ZSB0aGUgb24gZGlzayBzdGF0ZSB3aGVuIGxzbiB3YXMgdXBkYXRlZC4g Ki8KLQlpZiAoWExvZ1JlY1B0cklzSW52YWxpZChlbmRsc24pKQotCXsKLQkJUmVwbGljYXRpb25T bG90TWFya0RpcnR5KCk7Ci0JCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWlyZWRYbWluKGZh bHNlKTsKLQkJUmVwbGljYXRpb25TbG90c0NvbXB1dGVSZXF1aXJlZExTTigpOwotCQlSZXBsaWNh dGlvblNsb3RTYXZlKCk7Ci0JfQotCiAJUmVwbGljYXRpb25TbG90UmVsZWFzZSgpOwogCiAJLyog UmV0dXJuIHRoZSByZWFjaGVkIHBvc2l0aW9uLiAqLwoKYmFzZS1jb21taXQ6IDBjZTM4NzMwYWM3 MjAyOWYzZjJjOTVhZTgwYjQ0ZjViOTA2MGNiY2MKLS0gCjIuMTkuMQoK --=_dbebaff8668074c640bc6916df585dbe--