Subject: Re: confirmed flush lsn seems to be move backward in certain error cases

From: Tomas Vondra <[email-masked]>
Date: Tue, 11 Jun 2024 15:42:44 +0200

Hi,

On 6/11/24 10:39, Amit Kapila wrote:
> On Mon, Jun 10, 2024 at 7:24 PM vignesh C <[email-masked]> wrote:
>>
>> I have re-verified  the issue by running the tests in a loop of 150
>> times and found it to be working fine. Also patch applies neatly,
>> there was no pgindent issue and all the regression/tap tests run were
>> successful.
>>
> 
> Thanks, I have pushed the fix.
> 

Sorry for not responding to this thread earlier (two conferences in two
weeks), but isn't the pushed fix addressing a symptom instead of the
actual root cause?

Why should it be OK for the subscriber to confirm a flush LSN and then
later take that back and report a lower LSN? Seems somewhat against my
understanding of what "flush LSN" means.

The commit message explains this happens when the subscriber does not
need to do anything for - but then why shouldn't it just report the
prior LSN, in such cases?

I haven't looked into the details, but my concern is this removes an
useful assert, protecting us against certain type of bugs. And now we'll
just happily ignore them. Is that a good idea?

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Tom Lane <[email-masked]>
Date: Tue, 11 Jun 2024 15:14:49 -0400

> prior LSN, in such cases?

Yeah, I was wondering about that too when I saw the commit go by.

> just happily ignore them. Is that a good idea?

If we think this is a real protection, then it shouldn't be an Assert
anyway, because it will not protect production systems that way.
It needs to be regular test-and-elog.  Or maybe test-and-ignore-the-
bogus-value?  If you want to take this seriously then you need to
define a recovery procedure after the problem is detected.

			regards, tom lane

---

From: Amit Kapila <[email-masked]>
Date: Wed, 12 Jun 2024 09:24:24 +0530

> understanding of what "flush LSN" means.

The reason is that the subscriber doesn't persistently store/advance
the LSN for which it doesn't have to do anything like DDLs. But still,
the subscriber has to acknowledge such LSNs for synchronous
replication. We have comments/code at various places to deal with this
[1][2]. Now, after the restart, the subscriber won't know of such LSNs
so it will use its origin LSN which is the LSN of the last applied
transaction (and it can be before the LSN that last time the
subscriber had acknowledged). I had once thought to persist such LSNs
on subscriber by advancing the origin but that could be overhead in
certain workloads where logical decoding doesn't yield anything
meaningful for subscribers. So it needs more thought.

> prior LSN, in such cases?

It is required for synchronous replication.

> just happily ignore them. Is that a good idea?

The assert was added in this release. I was also having the same
understanding as yours which is why I added it. However, the case
presented by Vignesh has revealed that I was wrong.

-- 
With Regards,
Amit Kapila.