On Fri, May 23, 2025 at 12:15 PM shveta malik <shveta.ma...@gmail.com> wrote:
>
> Thanks you for v31 patch-set. Please find few comments on patch001:
>
> 1)
>
> wait_for_local_flush:
>
> + if (data->last_recv_time &&
> + TimestampDifferenceExceeds(data->flushpos_update_time,
> + data->last_recv_time, WalWriterDelay))
> + {
> + XLogRecPtr writepos;
> + XLogRecPtr flushpos;
> + bool have_pending_txes;
> +
> + /* Fetch the latest remote flush position */
> + get_flush_position(&writepos, &flushpos, &have_pending_txes);
> +
> + if (flushpos > last_flushpos)
> + last_flushpos = flushpos;
> +
> + data->flushpos_update_time = data->last_recv_time;
> + }
>
> We should only get new flush-position, if 'last_flushpos' is still
> lesser than 'data->remote_lsn'. Since 'last_flushpos' is also updated
> by 'send_feedback' and we do not update 'data->flushpos_update_time'
> there, it is possible that we have latest flush position but still
> TimestampDifferenceExceeds gives 'true', making it re-read the flush
> position unnecessarily.
>
> Having said that, I think the correct way will be to move
> 'flushpos_update_time' out of RetainConflictInfoData() similar to
> last_flushpos. Let it be a static variable, then we can update it in
> send_feedback as well.
>
But then we may sometimes need to call GetCurrentTimestamp to set its
value, which is not required now. Because we expect to skip this check
only in case when we are frequently applying the changes.
--
With Regards,
Amit Kapila.
