On Mon, Sep 1, 2025 at 5:45 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> >
> > Here is V70 patch set.
> >
>
> The patch v70-0001 looks good to me. Verified, all the old issues are resolved.
>
> Will resume review of v70-0002 now.
>
Please find a few comments on v70-0002:
1)
- * Note: Retention won't be resumed automatically. The user must manually
- * disable retain_dead_tuples and re-enable it after confirming that the
- * replication slot maintained by the launcher has been dropped.
+ * The retention will resume automatically if the worker has confirmed that the
+ * retention duration is now within the max_retention_duration.
Do we need this comment atop stop as it does not directly concern
stop? Isn't the details regarding RDT_RESUME_CONFLICT_INFO_RETENTION
in the file-header section sufficient?
2)
+ /* Stop retention if not yet */
+ if (MySubscription->retentionactive)
+ {
+ rdt_data->phase = RDT_STOP_CONFLICT_INFO_RETENTION;
- /* process the next phase */
- process_rdt_phase_transition(rdt_data, false);
+ /* process the next phase */
+ process_rdt_phase_transition(rdt_data, false);
+ }
+
+ reset_retention_data_fields(rdt_data);
should_stop_conflict_info_retention() does reset_retention_data_fields
everytime when wait-time exceeds the limit, and when it actually stops
i.e. calls stop_conflict_info_retention through phase change; the stop
function also does reset_retention_data_fields and calls
process_rdt_phase_transition. Can we optimize this code part by
consolidating the reset_retention_data_fields() and
process_rdt_phase_transition() calls into
should_stop_conflict_info_retention() itself, eliminating redundancy?
3)
Shall we update 035_conflicts.pl to have a resume test by setting
max_retention_duration to 0 after stop-retention test?
4)
+ subscription. The retention will be automatically resumed
once at least
+ one apply worker confirms that the retention duration is within the
+ specified limit, or a new subscription is created with
+ <literal>retain_dead_tuples = true</literal>, or the user manually
+ re-enables <literal>retain_dead_tuples</literal>.
Shall we rephrase it slightly to:
Retention will automatically resume when at least one apply worker
confirms that the retention duration is within the specified limit, or
when a new subscription is created with <literal>retain_dead_tuples =
true</literal>. Alternatively, retention can be manually
resumed by re-enabling <literal>retain_dead_tuples</literal>.
thanks
Shveta
