diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml index c1f6f1aaa8..dfbff3de02 100644 --- a/doc/src/sgml/logical-replication.sgml +++ b/doc/src/sgml/logical-replication.sgml @@ -1585,17 +1585,17 @@ test_sub=# SELECT * FROM t1 ORDER BY id; Additional logging is triggered in the following conflict - scenarios: + cases: insert_exists Inserting a row that violates a NOT DEFERRABLE - unique constraint. Note that to obtain the origin and commit - timestamp details of the conflicting key in the log, ensure that + unique constraint. Note that to log the origin and commit + timestamp details of the conflicting key, track_commit_timestamp - is enabled. In this scenario, an error will be raised until the + should be enabled. In this case, an error will be raised until the conflict is resolved manually. @@ -1605,10 +1605,10 @@ test_sub=# SELECT * FROM t1 ORDER BY id; The updated value of a row violates a NOT DEFERRABLE - unique constraint. Note that to obtain the origin and commit - timestamp details of the conflicting key in the log, ensure that + unique constraint. Note that to log the origin and commit + timestamp details of the conflicting key, track_commit_timestamp - is enabled. In this scenario, an error will be raised until the + is enabled. In this case, an error will be raised until the conflict is resolved manually. Note that when updating a partitioned table, if the updated row value satisfies another partition constraint resulting in the row being inserted into a @@ -1720,10 +1720,10 @@ CONTEXT: processing remote data for replication origin "pg_16395" during "INSER commit timestamp can be seen in the DETAIL line of the log. But note that this information is only available when track_commit_timestamp - is enabled. Users can use these information to make decisions on whether to - retain the local change or adopt the remote alteration. For instance, the - DETAIL line in above log indicates that the existing row was - modified by a local change, users can manually perform a remote-change-win + is enabled. Users can use this information to decide whether to retain the + local change or adopt the remote alteration. For instance, the + DETAIL line in the above log indicates that the existing + row was modified locally. Users can manually perform a remote-change-win resolution by deleting the local row. diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index ad3eda1459..34050a3bae 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -475,13 +475,13 @@ retry: } /* - * Find the tuple that violates the passed in unique index constraint - * (conflictindex). + * Find the tuple that violates the passed unique index (conflictindex). * - * If no conflict is found, return false and set *conflictslot to NULL. - * Otherwise return true, and the conflicting tuple is locked and returned in - * *conflictslot. The lock is essential to allow retrying to find the - * conflicting tuple in case the tuple is concurrently deleted. + * If the conflicting tuple is found return true, otherwise false. + * + * We lock the tuple to avoid getting it deleted before the caller can fetch + * the required information. Note that if the tuple is deleted before a lock + * is acquired, we will retry to find the conflicting tuple again. */ static bool FindConflictTuple(ResultRelInfo *resultRelInfo, EState *estate, @@ -528,25 +528,15 @@ retry: } /* - * Re-check all the unique indexes in 'recheckIndexes' to see if there are - * potential conflicts with the tuple in 'slot'. - * - * This function is invoked after inserting or updating a tuple that detected - * potential conflict tuples. It attempts to find the tuple that conflicts with - * the provided tuple. This operation may seem redundant with the unique - * violation check of indexam, but since we call this function only when we are - * detecting conflict in logical replication and encountering potential - * conflicts with any unique index constraints (which should not be frequent), - * so it's ok. Moreover, upon detecting a conflict, we will report an ERROR and - * restart the logical replication, so the additional cost of finding the tuple - * should be acceptable. + * Check all the unique indexes in 'recheckIndexes' for conflict with the + * tuple in 'slot' and report if found. */ static void -ReCheckConflictIndexes(ResultRelInfo *resultRelInfo, EState *estate, +CheckAndReportConflict(ResultRelInfo *resultRelInfo, EState *estate, ConflictType type, List *recheckIndexes, TupleTableSlot *slot) { - /* Re-check all the unique indexes for potential conflicts */ + /* Check all the unique indexes for a conflict */ foreach_oid(uniqueidx, resultRelInfo->ri_onConflictArbiterIndexes) { TupleTableSlot *conflictslot; @@ -622,17 +612,22 @@ ExecSimpleRelationInsert(ResultRelInfo *resultRelInfo, conflictindexes, false); /* - * Rechecks the conflict indexes to fetch the conflicting local tuple + * Checks the conflict indexes to fetch the conflicting local tuple * and reports the conflict. We perform this check here, instead of - * perform an additional index scan before the actual insertion and + * performing an additional index scan before the actual insertion and * reporting the conflict if any conflicting tuples are found. This is * to avoid the overhead of executing the extra scan for each INSERT * operation, even when no conflict arises, which could introduce * significant overhead to replication, particularly in cases where * conflicts are rare. + * + * XXX OTOH, this could lead to clean-up effort for dead tuples added + * in heap and index in case of conflicts. But as conflicts shouldn't + * be a frequent thing so we preferred to save the performance overhead + * of extra scan before each insertion. */ if (conflict) - ReCheckConflictIndexes(resultRelInfo, estate, CT_INSERT_EXISTS, + CheckAndReportConflict(resultRelInfo, estate, CT_INSERT_EXISTS, recheckIndexes, slot); /* AFTER ROW INSERT Triggers */ @@ -710,12 +705,12 @@ ExecSimpleRelationUpdate(ResultRelInfo *resultRelInfo, (update_indexes == TU_Summarizing)); /* - * Refer to the comments above the call to ReCheckConflictIndexes() in + * Refer to the comments above the call to CheckAndReportConflict() in * ExecSimpleRelationInsert to understand why this check is done at * this point. */ if (conflict) - ReCheckConflictIndexes(resultRelInfo, estate, CT_UPDATE_EXISTS, + CheckAndReportConflict(resultRelInfo, estate, CT_UPDATE_EXISTS, recheckIndexes, slot); /* AFTER ROW UPDATE Triggers */ diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index b1dc04ec7e..bff98a236d 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -2668,8 +2668,7 @@ apply_handle_update_internal(ApplyExecutionData *edata, TimestampTz localts; /* - * Check whether the local tuple was modified by a different origin. - * If detected, report the conflict. + * Report the conflict if the tuple was modified by a different origin. */ if (GetTupleCommitTs(localslot, &localxmin, &localorigin, &localts) && localorigin != replorigin_session_origin) @@ -2825,8 +2824,7 @@ apply_handle_delete_internal(ApplyExecutionData *edata, TimestampTz localts; /* - * Check whether the local tuple was modified by a different origin. - * If detected, report the conflict. + * Report the conflict if the tuple was modified by a different origin. */ if (GetTupleCommitTs(localslot, &localxmin, &localorigin, &localts) && localorigin != replorigin_session_origin) @@ -3038,8 +3036,7 @@ apply_handle_tuple_routing(ApplyExecutionData *edata, } /* - * Check whether the local tuple was modified by a different - * origin. If detected, report the conflict. + * Report the conflict if the tuple was modified by a different origin. */ if (GetTupleCommitTs(localslot, &localxmin, &localorigin, &localts) && localorigin != replorigin_session_origin)