RE: Conflict detection for update_deleted in logical replication

On Friday, January 3, 2025 1:53 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Wed, Dec 25, 2024 at 8:13 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Attach the new version patch set which addressed all other comments.
> >
> 
> Some more miscellaneous comments:

Thanks for the comments!

> =============================
> 1.
> @@ -1431,9 +1431,9 @@ RecordTransactionCommit(void)
>   * modifying it.  This makes checkpoint's determination of which xacts
>   * are delaying the checkpoint a bit fuzzy, but it doesn't matter.
>   */
> - Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0);
> + Assert((MyProc->delayChkptFlags & DELAY_CHKPT_IN_COMMIT) == 0);
>   START_CRIT_SECTION();
> - MyProc->delayChkptFlags |= DELAY_CHKPT_START;
> + MyProc->delayChkptFlags |= DELAY_CHKPT_IN_COMMIT;
> 
>   /*
>   * Insert the commit XLOG record.
> @@ -1536,7 +1536,7 @@ RecordTransactionCommit(void)
>   */
>   if (markXidCommitted)
>   {
> - MyProc->delayChkptFlags &= ~DELAY_CHKPT_START;
> + MyProc->delayChkptFlags &= ~DELAY_CHKPT_IN_COMMIT;
>   END_CRIT_SECTION();
> 
> The comments related to this change should be updated in EndPrepare()
> and RecordTransactionCommitPrepared(). They still refer to the
> DELAY_CHKPT_START flag. We should update the comments explaining why
> a
> similar change is not required for prepare or commit_prepare, if there
> is one.

After considering more, I think we need to use the new flag in
RecordTransactionCommitPrepared() as well, because it is assigned a commit
timestamp and would be replicated as normal transaction if sub's two_phase is
not enabled.

> 3.
> +FindMostRecentlyDeletedTupleInfo(Relation rel, TupleTableSlot *searchslot,
> + TransactionId *delete_xid,
> + RepOriginId *delete_origin,
> + TimestampTz *delete_time)
> ...
> ...
> + /* Try to find the tuple */
> + while (table_scan_getnextslot(scan, ForwardScanDirection, scanslot))
> + {
> + bool dead = false;
> + TransactionId xmax;
> + TimestampTz localts;
> + RepOriginId localorigin;
> +
> + if (!tuples_equal(scanslot, searchslot, eq, indexbitmap))
> + continue;
> +
> + tuple = ExecFetchSlotHeapTuple(scanslot, false, NULL);
> + buf = hslot->buffer;
> +
> + LockBuffer(buf, BUFFER_LOCK_SHARE);
> +
> + if (HeapTupleSatisfiesVacuum(tuple, oldestXmin, buf) ==
> HEAPTUPLE_RECENTLY_DEAD)
> + dead = true;
> +
> + LockBuffer(buf, BUFFER_LOCK_UNLOCK);
> +
> + if (!dead)
> + continue;
> 
> Why do we need to check only for HEAPTUPLE_RECENTLY_DEAD and not
> HEAPTUPLE_DEAD? IIUC, we came here because we couldn't find the live
> tuple, now whether the tuple is DEAD or RECENTLY_DEAD, why should it
> matter to detect update_delete conflict?

The HEAPTUPLE_DEAD could indicate tuples whose inserting transaction was
aborted, in which case we could not get the commit timestamp or origin for the
transaction. Or it could indicate tuples deleted by a transaction older than
oldestXmin(we would take the new replication slot's xmin into account when
computing this value), which means any subsequent transaction would have commit
timestamp later than that old delete transaction, so I think it's OK to ignore
this dead tuple and even detect update_missing because the resolution is to
apply the subsequent UPDATEs anyway (assuming we are using last update win
strategy). I added some comments along these lines in the patch.

> 
> 5.
> +
> +      <varlistentry
> id="sql-createsubscription-params-with-detect-update-deleted">
> +        <term><literal>detect_update_deleted</literal>
> (<type>boolean</type>)</term>
> +        <listitem>
> +         <para>
> +          Specifies whether the detection of <xref
> linkend="conflict-update-deleted"/>
> +          is enabled. The default is <literal>false</literal>. If set to
> +          true, the dead tuples on the subscriber that are still useful for
> +          detecting <xref linkend="conflict-update-deleted"/>
> +          are retained,
> 
> One of the purposes of retaining dead tuples is to detect
> update_delete conflict. But, I also see the following in 0001's commit
> message: "Since the mechanism relies on a single replication slot, it
> not only assists in retaining dead tuples but also preserves commit
> timestamps and origin data. These information will be displayed in the
> additional logs generated for logical replication conflicts.
> Furthermore, the preserved commit timestamps and origin data are
> essential for consistently detecting update_origin_differs conflicts."
> which indicates there are other cases where retaining dead tuples can
> help. So, I was thinking about whether to name this new option as
> retain_dead_tuples or something along those lines?

I used the retain_conflict_info in this version as it looks more general and we
are already using similar name in patch(RetainConflictInfoData), but we can
change it later if people have better ideas.

Attached the V19 patch which addressed comments in [1][2][3][4][5][6][7].

The point to remove FullTransactionID have not been addressed as the discussion is
still on going.

[1] https://www.postgresql.org/message-id/CAA4eK1JpJPQAPcnrVBNJ8CMaFfO9A9P6-DmXbwSfMAVrjMi5Qw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm1_V%2BWPThOkZy%2BR9_sWgHH5H6hN6UtEmq4Mj3QbUc3G8g%40mail.gmail.com
[3] https://www.postgresql.org/message-id/CAA4eK1JkYpDUCFgGNEg%3DO4m6XjQ-9oohxrohgpockKuy7eo9gA%40mail.gmail.com
[4] https://www.postgresql.org/message-id/CALDaNm3whh00AN58Azsps6%2BNLsYgqL6w2hz6wmcqSw5uiYqAbA%40mail.gmail.com
[5] https://www.postgresql.org/message-id/CALDaNm08M6CRZkK%3DBtVfS1%3D%2BzV2Qayg%2BfnYXQEPBiMOQ39m6sg%40mail.gmail.com
[6] https://www.postgresql.org/message-id/CAA4eK1%2BXfGqYRJPFk0wUHAh3mkJ59Tj2q%2BgXctyk7SKiASHgFA%40mail.gmail.com
[7] https://www.postgresql.org/message-id/CAA4eK1%2BdAJNPJWd_%2BOR7s%2B4rzSs48Jaoa2%2B0WNe%2B%3D9VQrCh4_A%40mail.gmail.com

Best Regards,
Hou zj