On Tue, May 27, 2025 at 3:59 PM shveta malik <shveta.ma...@gmail.com> wrote:
>
> On Mon, May 26, 2025 at 12:46 PM Zhijie Hou (Fujitsu)
> <houzj.f...@fujitsu.com> wrote:
> >
> > Attaching the V32 patch set which addressed comments in [1]~[5].
>
> Thanks for the patch, I am still reviewing the patches, please find
> few trivial comments for patch001:
>
> 1)
>
> + FullTransactionId last_phase_at; /* publisher transaction ID that must
> + * be awaited to complete before
> + * entering the final phase
> + * (RCI_WAIT_FOR_LOCAL_FLUSH) */
>
> 'last_phase_at' seems like we are talking about the phase in the past.
> (similar to 'last' in last_recv_time).
> Perhaps we should name it as 'final_phase_at'
>
>
> 2)
> RetainConflictInfoData data = {0};
>
> We can change this name as well to rci_data.
>
> 3)
> + /*
> + * Compute FullTransactionId for the oldest running transaction ID. This
> + * handles the case where transaction ID wraparound has occurred.
> + */
> + full_xid = FullTransactionIdFromAllowableAt(next_full_xid,
> oldest_running_xid);
>
> Shall we name it to full_oldest_xid for better clarity?
>
>
> 4)
> + /*
> + * Update and check the remote flush position if we are applying changes
> + * in a loop. This is done at most once per WalWriterDelay to avoid
> + * performing costy operations in get_flush_position() too frequently
> + * during change application.
> + */
> + if (last_flushpos < rci_data->remote_lsn && rci_data->last_recv_time &&
> + TimestampDifferenceExceeds(rci_data->flushpos_update_time,
> + rci_data->last_recv_time, WalWriterDelay))
>
> a) costy --> costly
>
Please find few comments in docs of v32-003:
1)
logical-replication.sgml:
<para>
<link
linkend="guc-max-replication-slots"><varname>max_replication_slots</varname></link>
must be set to at least the number of subscriptions expected to connect,
- plus some reserve for table synchronization.
+ plus some reserve for table synchronization and one if
+ <link
linkend="sql-createsubscription-params-with-retain-conflict-info"><literal>retain_conflict_info</literal></link>
is enabled.
</para>
Above doc is updated under Publishers section:
<sect2 id="logical-replication-config-publisher">
<title>Publishers</title>
But the conflict-slot is created on subscriber, so this info shall be
moved to subscriber. But subscriber currently does not have
'max_replication_slots' parameter under it. But I guess with
conflict-slot created on subscribers, we need to have that parameter
under 'Subscribers' too.
2)
catalogs.sgml:
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>subretainconflictinfo</structfield> <type>bool</type>
+ </para>
+ <para>
+ If true, the detection of <xref linkend="conflict-update-deleted"/> is
+ enabled and the information (e.g., dead tuples, commit timestamps, and
+ origins) on the subscriber that is still useful for conflict detection
+ is retained.
+ </para></entry>
+ </row>
+
2a) In absence of rest of the patches atop 3rd patch, this failed to
compile due to missing xref link. Error:
>element xref: validity error : IDREF attribute linked references an unknown ID
>"conflict-update-deleted"
2b) Also, if 'subretainconflictinfo' is true, IMO, it does not enable
the update_deleted detection, it just provides information which can
be used for detection. We should rephrase this doc.
3)
<xref linkend="conflict-update-deleted"/> is used in
create_subscription.sgml as well. That too needs correction.
4)
+ if (!sub_enabled)
+ ereport(WARNING,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("information for detecting conflicts cannot be purged when
the subscription is disabled"));
WARNING is good but not enough to clarify the problem and our
recommendation in such a case. Shall we update the docs as well as
explaining that such a situation may result in system bloat and thus
if subscription is disabled for longer, it is good to have
retain_conflict_info disabled as well.
thanks
Shveta
