On Fri, Jan 9, 2026 at 4:49 PM Dilip Kumar <[email protected]> wrote: > > On Thu, Jan 8, 2026 at 12:30 PM shveta malik <[email protected]> wrote: > > > > On Wed, Jan 7, 2026 at 12:12 PM shveta malik <[email protected]> wrote: > > > > > > Hi Dilip, > > > Please find a few comments on v19-001: > > > > > > > Please find a few comments for v19-002's part I have reviewed so far: > > > > 1) > > ReportApplyConflict: > > + if ((dest & CONFLICT_LOG_DEST_TABLE) != 0) > > + if ((dest & CONFLICT_LOG_DEST_LOG) != 0) > > > > We can use IsSet everywhere > > IMHO in subscriptioncmd.c we very frequently use the bitwise operation > so it has IsSet declared, I am not really sure do we want to define > IsSet in this function as we are using it just 2 places. > > > 2) > > GetConflictLogTableInfo > > This function gets dest and opens table, shall we rename to : > > GetConflictLogDestAndTable > > Yeah make sense. > > > 3) > > GetConflictLogTableInfo: > > + *log_dest = GetLogDestination(MySubscription->conflictlogdest); > > + conflictlogrelid = MySubscription->conflictlogrelid; > > + > > + /* If destination is 'log' only, no table to open. */ > > + if (*log_dest == CONFLICT_LOG_DEST_LOG) > > + return NULL; > > + > > + Assert(OidIsValid(conflictlogrelid)); > > > > We don't need to fetch conflictlogrelid until after 'if (*log_dest == > > CONFLICT_LOG_DEST_LOG)' check. We shall move it after the 'if' check. > > Done > > > 4) > > GetConflictLogTableInfo: > > + /* Conflict log table is dropped or not accessible. */ > > + if (conflictlogrel == NULL) > > + ereport(WARNING, > > + (errcode(ERRCODE_UNDEFINED_TABLE), > > + errmsg("conflict log table with OID %u does not exist", > > + conflictlogrelid))); > > > > Shall we replace it with elog(ERROR)? IMO, it should never happen and > > if it happens, we should raise it as an internal error as we do for > > various other cases. > > Right, now its internal table so we should make it elog(ERROR) > > > > > 5) > > ReportApplyConflict(): > > > > Currently the function structure is: > > > > /* Insert to table if destination is 'table' or 'all' */ > > if ((dest & CONFLICT_LOG_DEST_TABLE) != 0) > > > > /* Decide what detail to show in server logs. */ > > if ((dest & CONFLICT_LOG_DEST_LOG) != 0) > > else <table-only: put reduced info in log> > > > > > > It will be good to make it: > > > > /* > > * Insert to table if destination is 'table' or 'all' and > > * also log the error msg to serverlog > > */ > > if ((dest & CONFLICT_LOG_DEST_TABLE) != 0) > > {...} > > else <CONFLICT_LOG_DEST_LOG case> > > {log complete detail} > > I did not understand this, I mean we can not put the "log complete > detail" case in the else part, because we might have to log the > complete detail along with the table if the destination is "all", are > you suggesting something else?
Okay, I see it now. I missed the 'all' part earlier. Thanks for pointing it out. The current implementation is good. > > > > > 6) > > tuple_table_slot_to_indextup_json: > > + tuple = heap_form_tuple(tupdesc, values, isnull); > > > > Do we need to do: heap_freetuple at the end? > > Yeah better we do that, instead of assuming short lived memory context. > > > -- > Regards, > Dilip Kumar > Google
