On Fri, May 23, 2025 at 2:45 PM shveta malik wrote:
>
> Thanks you for v31 patch-set. Please find few comments on patch001:
Thanks for the comments.
>
> 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.
I added the check to update the flush when last_flushpos is behind.
But I tend to avoid adding more static variables if possible.
The flushpos_update_time is only used to prevent fetching flush position too
frequently which is specific to RCI logic. And even if in some cases, we might
have already updated the flush position in send_feedback(), I feel it's not a
big issue as the new flush position update logic is only used when applying
changes in a loop where send_feedback() is rarely invoked.
> 4)
> Everywhere we are using variable name as 'data', it is a very generic
> name. Shall we change it to 'conflict_info_data' or
> 'retain_conf_info_data'?
I changed them to rci_data which is shorter.
Best Regards,
Hou zj
