On Wed, Oct 7, 2026 at 10:21 PM Zhijie Hou <[email protected]> wrote:
>
> I've started reviewing the 0001 patch and would like to suggest some
> refactoring
> to simplify the code in ReportApplyConflict(). The idea is to move the
> fallback
> logic into get_conflictlog_dest_and_table() so the caller's code looks
> cleaner.
>
> And I think we could build the errdetail for both the table and
> logfile cases and call ereport() only once, to reduce the duplicated code.
>
> Please check the attached follow-up patches (on top of 0001), I hope it makes
> the code better.
>
I also think so but we can go a bit further to simplify this part by
removing the two booleans and ERROR level check inside the
ReportApplyConflict() function. See attached built atop your patch.
Additionally, tried improving the comments atop
get_conflictlog_dest_and_table() in the attached.
--
With Regards,
Amit Kapila.
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index 527ee2b27a5..d5146d21bc4 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -131,7 +131,8 @@ static const char *const ConflictTypeNames[] = {
[CT_MULTIPLE_UNIQUE_CONFLICTS] = "multiple_unique_conflicts"
};
-static Relation get_conflictlog_dest_and_table(ConflictLogDest *log_dest);
+static Relation get_conflictlog_dest_and_table(int elevel,
+
ConflictLogDest *log_dest);
static int errcode_apply_conflict(ConflictType type);
static void errdetail_apply_conflict(EState *estate,
ResultRelInfo *relinfo,
@@ -335,25 +336,13 @@ ReportApplyConflict(EState *estate, ResultRelInfo
*relinfo, int elevel,
TupleTableSlot *remoteslot, List
*conflicttuples)
{
Relation localrel = relinfo->ri_RelationDesc;
- ConflictLogDest dest = CONFLICT_LOG_DEST_LOG;
- Relation conflictlogrel = NULL;
- bool log_dest_table;
- bool log_dest_logfile;
+ ConflictLogDest dest;
+ Relation conflictlogrel;
StringInfoData err_detail;
pgstat_report_subscription_conflict(MySubscription->oid, type);
- /*
- * Only LOG-level conflicts (i.e. resolved conflicts where the
transaction
- * continues) are recorded in the conflict log table. ERROR-level
- * conflicts halt replication and abort the transaction, so they are
- * always reported exclusively to the server log.
- */
- if (elevel < ERROR)
- conflictlogrel = get_conflictlog_dest_and_table(&dest);
-
- log_dest_table = CONFLICTS_LOGGED_TO_TABLE(dest);
- log_dest_logfile = CONFLICTS_LOGGED_TO_LOG(dest);
+ conflictlogrel = get_conflictlog_dest_and_table(elevel, &dest);
initStringInfo(&err_detail);
@@ -363,7 +352,7 @@ ReportApplyConflict(EState *estate, ResultRelInfo *relinfo,
int elevel,
* Otherwise (table-only for LOG-level conflicts), emit a shorter
message
* noting that the details are captured in the conflict log table.
*/
- if (log_dest_logfile)
+ if (CONFLICTS_LOGGED_TO_LOG(dest))
{
/* Form errdetail message by combining conflicting tuples
information. */
if (conflicttuples != NIL)
@@ -388,7 +377,7 @@ ReportApplyConflict(EState *estate, ResultRelInfo *relinfo,
int elevel,
&err_detail);
}
}
- else if (log_dest_table)
+ else
{
appendStringInfo(&err_detail,
_("Conflict details are logged
to the conflict log table: %s.%s"),
@@ -408,11 +397,8 @@ ReportApplyConflict(EState *estate, ResultRelInfo
*relinfo, int elevel,
* Insert into the conflict log table for LOG-level conflicts if
* requested.
*/
- if (log_dest_table)
+ if (conflictlogrel)
{
- Assert(conflictlogrel != NULL);
- Assert(elevel < ERROR);
-
insert_conflict_log_tuple(estate,
relinfo->ri_RelationDesc,
conflictlogrel,
@@ -457,22 +443,28 @@ InitConflictIndexes(ResultRelInfo *relInfo)
/*
* get_conflictlog_dest_and_table
*
- * Fetches conflict logging metadata from the cached MySubscription pointer.
- * Sets the destination enum in *log_dest and, if a table is one of the
- * destinations, opens and returns the relation handle for the conflict log
- * table.
+ * Decides where a conflict of the given elevel is to be reported, based on
+ * the subscription's conflict_log_destination. Sets *log_dest to the
+ * effective destination and, if that includes the conflict log table, returns
+ * the table opened for insertion. Otherwise returns NULL.
*
- * The table is opened with try_table_open(), so NULL is returned if the
- * conflict log table has been dropped concurrently (e.g. by an ALTER
- * SUBSCRIPTION that changed conflict_log_destination). In that case
- * *log_dest falls back to server-log reporting.
+ * Only LOG-level conflicts (i.e. resolved conflicts where the transaction
+ * continues) are recorded in the conflict log table. ERROR-level conflicts
+ * halt replication and abort the transaction, so they are always reported
+ * exclusively to the server log.
*/
static Relation
-get_conflictlog_dest_and_table(ConflictLogDest *log_dest)
+get_conflictlog_dest_and_table(int elevel, ConflictLogDest *log_dest)
{
Relation conflictlogrel;
Oid conflictlogrelid;
+ if (elevel >= ERROR)
+ {
+ *log_dest = CONFLICT_LOG_DEST_LOG;
+ return NULL;
+ }
+
/*
* Convert the text log destination to the internal enum.
MySubscription
* already contains the data from pg_subscription.
@@ -488,10 +480,10 @@ get_conflictlog_dest_and_table(ConflictLogDest *log_dest)
Assert(OidIsValid(conflictlogrelid));
/*
- * Use try_table_open(): the table may have been dropped concurrently
by an
- * ALTER SUBSCRIPTION that changed conflict_log_destination. Returning
- * NULL and tell the caller to fall back to the server log instead of
- * failing.
+ * Use try_table_open() because the table may have been dropped
+ * concurrently by an ALTER SUBSCRIPTION that changed
+ * conflict_log_destination. In that case, fall back to the server log
+ * instead of failing.
*/
conflictlogrel = try_table_open(conflictlogrelid, RowExclusiveLock);
