From: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> |
---|---|
To: | a(dot)kondratov(at)postgrespro(dot)ru |
Cc: | pgsql-hackers(at)postgresql(dot)org, michael(at)paquier(dot)xyz, simon(at)2ndquadrant(dot)com |
Subject: | Re: Physical replication slot advance is not persistent |
Date: | 2019-12-26 08:33:49 |
Message-ID: | 20191226.173349.2159296357637287391.horikyota.ntt@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
At Wed, 25 Dec 2019 20:28:04 +0300, Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru> wrote in
> > Yep, it helps with physical replication slot persistence after
> > advance, but the whole validation (moveto <= endlsn) does not make
> > sense for me. The value of moveto should be >= than minlsn ==
> > confirmed_flush / restart_lsn, while endlsn == retlsn is also always
> > initialized with confirmed_flush / restart_lsn. Thus, your condition
> > seems to be true in any case, even if it was no-op one, which we were
> > intended to catch.
...
> If I get it correctly, then we already keep previous slot position in
> the minlsn, so we just have to compare endlsn with minlsn and treat
> endlsn <= minlsn as a no-op without slot state flushing.
I think you're right about the condition. (endlsn cannot be less than
minlsn, though) But I came to think that we shouldn't use locations in
that decision.
> Attached is a patch that does this, so it fixes the bug without
> affecting any user-facing behavior. Detailed comment section and DEBUG
> output are also added. What do you think now?
>
> I have also forgotten to mention that all versions down to 11.0 should
> be affected with this bug.
pg_replication_slot_advance is the only caller of
pg_logical/physical_replication_slot_advacne so there's no apparent
determinant on who-does-what about dirtying and other housekeeping
calculation like *ComputeRequired*() functions, but the current shape
seems a kind of inconsistent between logical and physical.
I think pg_logaical/physical_replication_slot_advance should dirty the
slot if they actually changed anything. And
pg_replication_slot_advance should do the housekeeping if the slots
are dirtied. (Otherwise both the caller function should dirty the
slot in lieu of the two.)
The attached does that.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
0001-Make-sure-to-save-updated-physical-slot.patch | text/x-patch | 2.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Julien Rouhaud | 2019-12-26 08:51:23 | Re: Expose lock group leader pid in pg_stat_activity |
Previous Message | Guillaume Lelarge | 2019-12-26 08:08:08 | Re: Expose lock group leader pid in pg_stat_activity |