Re: Unnecessary confirm work on logical replication

On 4/11/23 14:58, Emre Hasegeli wrote:
>> In fact, the WAL sender always starts reading WAL from restart_lsn,
>> which in turn is always <= confirmed_flush_lsn. While reading WAL, WAL
>> sender may read XLOG_RUNNING_XACTS WAL record with lsn <=
>> confirmed_flush_lsn. While processing XLOG_RUNNING_XACTS record it may
>> update its restart_lsn and catalog_xmin with current_lsn = lsn fo
>> XLOG_RUNNING_XACTS record. In this situation current_lsn <=
>> confirmed_flush_lsn.
>
> This can only happen when the WAL sender is restarted. However in
> this case, the restart_lsn and catalog_xmin should have already been
> persisted by the previous run of the WAL sender.
>
> I still doubt these calls are necessary. I think there is a
> complicated chicken and egg problem here. Here is my logic:
>
> 1) LogicalConfirmReceivedLocation() is called explicitly when
> confirmed_flush is sent by the replication client.
>
> 2) LogicalConfirmReceivedLocation() is the only place that updates
> confirmed_flush.
>
> 3) The replication client can only send a confirmed_flush for a
> current_lsn it has already received.
>
> 4) These two functions have already run for any current_lsn the
> replication client has received.
>
> 5) These two functions call LogicalConfirmReceivedLocation() only if
> current_lsn <= confirmed_flush.
>
> Thank you for your patience.
>

Hi Emre,

I was going through my TODO list of messages, and I stumbled on this
thread from a couple months back. Do you still think this is something
we should do?

I see there was some discussion about whether this update is needed, or
whether current_lsn can ever be <= confirmed_flush_lsn. I think you may
be right this can't happen, but I wonder if we could verify that by an
assert in a convenient place (instead of just removing the update).

Also, did you try to quantify how expensive this is? The update seems
very cheap, but I guess you just noticed by eye-balling the code, which
is fine ofc. Even if it's cheap/not noticeable, it still may be worth
removing so as not to confuse people improving the code in the future.

regards

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