On Thu, Aug 13, 2026 at 3:22 PM Dilip Kumar <[email protected]> wrote: > > On Thu, Aug 13, 2026 at 11:10 AM shveta malik <[email protected]> wrote: > > > > On Wed, Aug 12, 2026 at 9:13 PM Dilip Kumar <[email protected]> wrote: > > > > > > > > > Changed. > > > > > > > Thanks Dilip. A few trivial comments on v69: > > > > 1) > > +insert_conflict_log_tuple(Relation conflictlogrel, HeapTuple tuple) > > +{ > > + Assert(tuple != NULL); > > + > > + heap_insert(conflictlogrel, tuple, > > + GetCurrentCommandId(true), 0, NULL); > > +} > > > > Should we get rid of this function now? > > a) It does not do anything extra other than heap_insert() > > b) There is only one caller. > > So I have merged prepare_conflict_log_tuple() code into the > insert_conflict_log_tuple() and kept the name as > insert_conflict_log_tuple() > > > 2) > > LOG: conflict detected on relation "public.tab1": conflict=delete_missing > > DETAIL: Conflict details are logged to the conflict log table: > > pg_conflict_log_16390 > > > > It will be good to specify schema qualified name here: > > pg_conflict.pg_conflict_log_16390, as we do at other places. > > Done > > > > > 3) > > The doc page shows: > > > > 29.8.1. Conflict logging > > 29.8.2. Table-based logging > > 29.8.3. File-based logging > > 29.8.4. Notes > > > > Should it be: > > 29.8.1. Conflict logging > > 29.8.1.1. Table-based logging > > 29.8.1.2. File-based logging > > 29.8.2. Notes > > > > What do you say? > > Yeah I have moved 29.8.1.1, Table-based logging and 29.8.1.2, > File-based logging section into the 29.8.1. Conflict logging section, > but from the main page I have only kept the link unabled for the > 29.8.1. Conflict logging this is same as how it is done for other > subsections, e.g. 29.14.3.1 etc.. > >
Thanks, a few trivial things: 1) Currently in '29.8. Conflicts', we have: 29.8.1.1. Table-based logging 29.8.1.2. File-based logging I feel '29.8.1.2. File-based logging' should come first (i.e. as 29.8.1.1) followed by 'Table-based' logging. Reasons: a) default should be explained first. b) Table-based logging refers to File-based logging, so the latter should be explained first. c) order in create-sub and 'Conflicts' page should be same. 2) tuple_table_slot_to_indextup_json() has this: + Assert(CheckRelationOidLockedByMe(indexid, RowExclusiveLock, true)); We shall add a comment similar to one atop build_index_value_desc(): * The caller must ensure that the index with the OID 'indexoid' is locked so * that ..... 3) +/* + * build_conflict_tupledesc + * + * Build and bless a tuple descriptor for the conflict log table based on the + * predefined LocalConflictSchema. + */ +static TupleDesc +build_conflict_tupledesc(void) It is a generic name, perhaps we should rename to build_local_conflicts_tupledesc, based on caller build_local_conflicts_json_array. (Do it if you agree, not manadatory). 4) It will be good to have 'Assert(conflictlogrel != NULL)' inside insert_conflict_log_tuple as well before we access conflictlogrel. ~~ I do not have any further comments. The patch is good otherwise. thanks Shveta
