MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_1e7043299a5e4d2debc497d43e49daef"
Content-Transfer-Encoding: 8bit
Date: Thu, 18 Jun 2020 21:46:28 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, andres@anarazel.de,
 dim@tapoueh.org, pgsql-hackers@postgresql.org, simon@2ndquadrant.com
Subject: Re: Physical replication slot advance is not persistent
In-Reply-To: <20200616072727.GA2361@paquier.xyz>
References: <20200609171904.kpltxxvjzislidks@alap3.anarazel.de>
 <82163ef4a380c5255e5004659d02559e@postgrespro.ru>
 <20200610065353.GA358163@paquier.xyz>
 <20200610.173844.78906193765823414.horikyota.ntt@gmail.com>
 <876a782d320a52b0c54d6d48372de17d@postgrespro.ru>
 <20200616072727.GA2361@paquier.xyz>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <0a14a258081aea401222498d402d129b@postgrespro.ru>
Precedence: bulk

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

On 2020-06-16 10:27, Michael Paquier wrote:
> On Wed, Jun 10, 2020 at 08:57:17PM +0300, Alexey Kondratov wrote:
>> New test reproduces this issue well. Left it running for a couple of 
>> hours
>> in repeat and it seems to be stable.
> 
> Thanks for testing.  I have been thinking about the minimum xmin and
> LSN computations on advancing, and actually I have switched the
> recomputing to be called at the end of pg_replication_slot_advance().
> This may be a waste if no advancing is done, but it could also be an
> advantage to enforce a recalculation of the thresholds for each
> function call.  And that's more consistent with the slot copy, drop
> and creation.
> 

Sorry for a bit late response, but I see a couple of issues with this 
modified version of the patch in addition to the waste call if no 
advancing is done, mentioned by you:

1. Both ReplicationSlotsComputeRequiredXmin() and 
ReplicationSlotsComputeRequiredLSN() may have already been done in the 
LogicalConfirmReceivedLocation() if it was a logical slot. It may be 
fine and almost costless to do it twice, but it looks untidy for me.

2. It seems that we do not need ReplicationSlotsComputeRequiredXmin() at 
all if it was a physical slot, since we do not modify xmin in the 
pg_physical_replication_slot_advance(), doesn't it?

That's why I wanted (somewhere around v5 of the patch in this thread) to 
move all dirtying and recomputing calls to the places, where xmin / lsn 
slot modifications are actually done — 
pg_physical_replication_slot_advance() and 
LogicalConfirmReceivedLocation(). LogicalConfirmReceivedLocation() 
already does this, so we only needed to teach 
pg_physical_replication_slot_advance() to do the same.

However, just noted that LogicalConfirmReceivedLocation() only does 
ReplicationSlotsComputeRequiredLSN() if updated_xmin flag was set, which 
looks wrong from my perspective, since updated_xmin and updated_restart 
flags are set separately.

That way, I would solve this all as per attached, which works well for 
me, but definitely worth of a better testing.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company
--=_1e7043299a5e4d2debc497d43e49daef
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=slots-advancing.diff
Content-Disposition: attachment;
 filename=slots-advancing.diff;
 size=1650

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvbG9naWNhbC5jIGIv
c3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9sb2dpY2FsLmMKaW5kZXggNjE5MDJiZTNi
MC4uMmNlYjA3ODQxOCAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNh
bC9sb2dpY2FsLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9sb2dpY2Fs
LmMKQEAgLTEwODEsOCArMTA4MSwxNCBAQCBMb2dpY2FsQ29uZmlybVJlY2VpdmVkTG9jYXRpb24o
WExvZ1JlY1B0ciBsc24pCiAJCQlTcGluTG9ja1JlbGVhc2UoJk15UmVwbGljYXRpb25TbG90LT5t
dXRleCk7CiAKIAkJCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWlyZWRYbWluKGZhbHNlKTsK
LQkJCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWlyZWRMU04oKTsKIAkJfQorCisJCS8qCisJ
CSAqIE5vdyByZWNvbXB1dGUgdGhlIG1pbmltdW0gcmVxdWlyZWQgTFNOIGFjcm9zcyBhbGwgc2xv
dHMgaWYKKwkJICogcmVzdGFydF9sc24gb2YgdGhlIHNsb3QgaGFzIGJlZW4gdXBkYXRlZC4KKwkJ
ICovCisJCWlmICh1cGRhdGVkX3Jlc3RhcnQpCisJCQlSZXBsaWNhdGlvblNsb3RzQ29tcHV0ZVJl
cXVpcmVkTFNOKCk7CiAJfQogCWVsc2UKIAl7CmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9yZXBs
aWNhdGlvbi9zbG90ZnVuY3MuYyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3RmdW5jcy5j
CmluZGV4IDA2ZTQ5NTVkZTcuLmQ5YzE5YzA3ZTUgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3Jl
cGxpY2F0aW9uL3Nsb3RmdW5jcy5jCisrKyBiL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL3Nsb3Rm
dW5jcy5jCkBAIC00MTksNiArNDE5LDEyIEBAIHBnX3BoeXNpY2FsX3JlcGxpY2F0aW9uX3Nsb3Rf
YWR2YW5jZShYTG9nUmVjUHRyIG1vdmV0bykKIAkJICogY3Jhc2gsIGJ1dCB0aGlzIG1ha2VzIHRo
ZSBkYXRhIGNvbnNpc3RlbnQgYWZ0ZXIgYSBjbGVhbiBzaHV0ZG93bi4KIAkJICovCiAJCVJlcGxp
Y2F0aW9uU2xvdE1hcmtEaXJ0eSgpOworCisJCS8qCisJCSAqIFJlY29tcHV0ZSB0aGUgbWluaW11
bSByZXF1aXJlZCBMU04gYWNyb3NzIGFsbCBzbG90cyB0byBhZGp1c3Qgd2l0aAorCQkgKiB0aGUg
YWR2YW5jaW5nIGRvbmUuCisJCSAqLworCQlSZXBsaWNhdGlvblNsb3RzQ29tcHV0ZVJlcXVpcmVk
TFNOKCk7CiAJfQogCiAJcmV0dXJuIHJldGxzbjsKQEAgLTYyMSwxMyArNjI3LDYgQEAgcGdfcmVw
bGljYXRpb25fc2xvdF9hZHZhbmNlKFBHX0ZVTkNUSU9OX0FSR1MpCiAJdmFsdWVzWzBdID0gTmFt
ZUdldERhdHVtKCZNeVJlcGxpY2F0aW9uU2xvdC0+ZGF0YS5uYW1lKTsKIAludWxsc1swXSA9IGZh
bHNlOwogCi0JLyoKLQkgKiBSZWNvbXB1dGUgdGhlIG1pbmltdW0gTFNOIGFuZCB4bWluIGFjcm9z
cyBhbGwgc2xvdHMgdG8gYWRqdXN0IHdpdGggdGhlCi0JICogYWR2YW5jaW5nIHBvdGVudGlhbGx5
IGRvbmUuCi0JICovCi0JUmVwbGljYXRpb25TbG90c0NvbXB1dGVSZXF1aXJlZFhtaW4oZmFsc2Up
OwotCVJlcGxpY2F0aW9uU2xvdHNDb21wdXRlUmVxdWlyZWRMU04oKTsKLQogCVJlcGxpY2F0aW9u
U2xvdFJlbGVhc2UoKTsKIAogCS8qIFJldHVybiB0aGUgcmVhY2hlZCBwb3NpdGlvbi4gKi8K
--=_1e7043299a5e4d2debc497d43e49daef--