On Tue, Aug 11, 2026 at 3:43 PM Amit Kapila <[email protected]> wrote: > > On Mon, Aug 10, 2026 at 6:45 PM Dilip Kumar <[email protected]> wrote: > > > > I will work on other follow-up patches soon. > > > > Few comments: > ============= > 1. > +InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tuple, > + const char *errcontext_str) > { > ... > ... > + /* > + * Set up an error context so that a failure to insert (e.g. an > + * out-of-space error) carries information identifying the conflict we > + * were trying to log. > + */ > + errcallback.callback = conflict_log_insert_errcontext; > + errcallback.arg = (void *) errcontext_str; > + errcallback.previous = error_context_stack; > + error_context_stack = &errcallback; > > Do we need this err context setup? I am seeing following LOGs after > setting conflict_log_destination='table': > > [logical replication apply worker] LOG: conflict detected on relation > "public.t1": conflict=delete_origin_differs > [logical replication apply worker] DETAIL: Conflict details are > logged to the conflict log table: pg_conflict_log_16392 > [logical replication apply worker] CONTEXT: processing remote data > for replication origin "pg_16392" during message type "DELETE" for > replication target relation "public.t1" in transaction 697, finished > at 0/017E3C60 > [logical replication apply worker] ERROR: testing insert failure > [logical replication apply worker] CONTEXT: while logging conflict > "delete_origin_differs" detected on relation "t1" > > We are reporting conflict "delete_origin_differs" twice. IIRC, this > err_context was required in an earlier approach where we use to delete > the ERROR reporting after insertion into table so that if there is > failure while inserting into table, we won't miss the conflict > information.
Yeah we don't need this now > 2. > + * The 'local_conflicts' column is typed as an array of JSON objects (json[]) > + * rather than a single json object to keep the exposed schema future-proof. > + * Although currently only resolved LOG-level conflicts (which involve a > single > + * local row) are recorded in this table, future capabilities (such as > conflict > + * resolution handlers or logging multi-row constraint conflicts) may need to > + * record multiple conflicting local rows for a single remote operation. > + * Defining it as an array from the start keeps the table schema stable and > + * avoids backward-incompatible schema changes or complex upgrade > handling later. > > Can we slightly shorten this comment as in attached? Done > 3. > +static Datum build_local_conflicts_json_array(List *conflicttuples); > +static HeapTuple prepare_conflict_log_tuple(EState *estate, Relation rel, > + Relation conflictlogrel, > + ConflictType conflict_type, > + TupleTableSlot *searchslot, > + List *conflicttuples, > + TupleTableSlot *remoteslot, > + char **errcontext_str); > +static void InsertConflictLogTuple(Relation conflictlogrel, HeapTuple tuple, > + const char *errcontext_str); > > Why the function naming for InsertConflictLogTuple in a different case > as compared to other static functions? It was previously an extern function, since it is now static, we changed the case. Fixed. > 4. "Handling Multi-row Conflicts: A single remote tuple may conflict > with multiple local tuples (e.g., in the case of > multiple_unique_conflicts). To handle this, the infrastructure creates > a single row in the conflict log table for each remote tuple. The > details of all conflicting local rows are aggregated into a single > JSON array in the local_conflicts column. > > Seeing above in the commit message, it seems you forgot to update the > commit message. Fixed -- Regards, Dilip Kumar Google
v68-0001-Implement-the-conflict-insertion-infrastructure-.patch
Description: Binary data
