Re: Conflict detection for update_deleted in logical replication

public inbox for [email protected]  
help / color / mirror / Atom feed

From: shveta malik <[email protected]>
To: Zhijie Hou (Fujitsu) <[email protected]>
Cc: Amit Kapila <[email protected]>
Cc: vignesh C <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: Nisha Moond <[email protected]>
Cc: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: shveta malik <[email protected]>
Subject: Re: Conflict detection for update_deleted in logical replication
Date: Thu, 29 May 2025 14:30:10 +0530
Message-ID: <CAJpy0uAJUTmSx7fAE3gbnBUzp9ZDOgkLrP5gdoysKUGbvf7vGg@mail.gmail.com> (raw)
In-Reply-To: <CAJpy0uBSsRuVOeuo-i8R_aO0CMiORHTnEBZ9z-TDq941WqhyLA@mail.gmail.com>
References: <CAA4eK1+r9V6DpH9gYRa2xOx167FapbuKdc4gESr8Etxpx2zrqw@mail.gmail.com>
	<CAA4eK1+VNaGi-GU6awgFKmTgidLTHo2HDuzV1+aT8sjn8QtPxg@mail.gmail.com>
	<OS0PR01MB5716B7983231B9DF3713EF2A9499A@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<CAA4eK1J+dmsB=od4pkGBjZkeurpWGuRXOeXBbNRbh3hVUr7DxQ@mail.gmail.com>
	<OS0PR01MB57161C2C4345AAFB3EA2E5309465A@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<CAJpy0uBSsRuVOeuo-i8R_aO0CMiORHTnEBZ9z-TDq941WqhyLA@mail.gmail.com>

On Tue, May 27, 2025 at 3:59 PM shveta malik <[email protected]> wrote:
>
> On Mon, May 26, 2025 at 12:46 PM Zhijie Hou (Fujitsu)
> <[email protected]> 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

view thread (375+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Conflict detection for update_deleted in logical replication
  In-Reply-To: <CAJpy0uAJUTmSx7fAE3gbnBUzp9ZDOgkLrP5gdoysKUGbvf7vGg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox