On Thu, Jan 8, 2026 at 12:30 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Wed, Jan 7, 2026 at 12:12 PM shveta malik <shveta.malik@gmail.com> 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?
>
> 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
