On Wed, Aug 14, 2024 at 8:05 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.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.