Re: Conflict detection and logging in logical replication

On Wed, Aug 14, 2024 at 8:05 AM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Here is the V14 patch.
>

Review comments:
1.
ReportApplyConflict()
{
...
+ ereport(elevel,
+ errcode(ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION),
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s",
+ get_namespace_name(RelationGetNamespace(localrel)),
...

Is it a good idea to use ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION for
all conflicts? I think it is okay to use for insert_exists and
update_exists. The other error codes to consider for conflicts other
than insert_exists and update_exists are
ERRCODE_T_R_SERIALIZATION_FAILURE, ERRCODE_CARDINALITY_VIOLATION,
ERRCODE_NO_DATA, ERRCODE_NO_DATA_FOUND,
ERRCODE_TRIGGERED_DATA_CHANGE_VIOLATION,
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE.

BTW, even for insert/update_exists, won't it better to use
ERRCODE_UNIQUE_VIOLATION?

2.
+build_tuple_value_details()
{
...
+ if (searchslot)
+ {
+ /*
+ * If a valid index OID is provided, build the replica identity key
+ * value string. Otherwise, construct the full tuple value for REPLICA
+ * IDENTITY FULL cases.
+ */

AFAICU, this can't happen for insert/update_exists. If so, we should
add an assert for those two conflict types.

3.
+build_tuple_value_details()
{
...
+    /*
+     * Although logical replication doesn't maintain the bitmap for the
+     * columns being inserted, we still use it to create 'modifiedCols'
+     * for consistency with other calls to ExecBuildSlotValueDescription.
+     */
+    modifiedCols = bms_union(ExecGetInsertedCols(relinfo, estate),
+                 ExecGetUpdatedCols(relinfo, estate));
+    desc = ExecBuildSlotValueDescription(relid, remoteslot, tupdesc,
+                       modifiedCols, 64);

Can we mention in the comments the reason for not including generated columns?

Apart from the above, the attached contains some cosmetic changes.

--
With Regards,
Amit Kapila.