On Tue, Dec 23, 2025 at 3:34 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Dec 22, 2025 at 4:01 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > Done in V15
>
> Thanks for the patches. A few comments on v15-002 for the part I have
> reviewed so far:
>
> 1)
> Defined twice:
>
> +#define MAX_LOCAL_CONFLICT_INFO_ATTRS 5
>
> +#define MAX_LOCAL_CONFLICT_INFO_ATTRS \
> + (sizeof(LocalConflictSchema) / sizeof(LocalConflictSchema[0]))
Fixed
> 2)
> GetConflictLogTableInfo:
> + *log_dest = GetLogDestination(MySubscription->logdestination);
> + conflictlogrelid = MySubscription->conflictrelid;
> +
> + /* If destination is 'log' only, no table to open. */
> + if (*log_dest == CONFLICT_LOG_DEST_LOG)
> + return NULL;
>
> We can get conflictlogrelid after the if-check for DEST_LOG.
I am planning to do some more refactoring considering other comments,
based on that we need to first get the destination so flow will be
like
{
....
conflictlogrel = GetConflictLogTableInfo(&dest);
if ((dest & CONFLICT_LOG_DEST_TABLE) != 0)
{
/* insert into table */
}
if ((dest & CONFLICT_LOG_DEST_LOG) != 0)
{
-- Prepare error details
-- Log with error detail
}
else
{
-- log basic information
}
> 3)
> In ReportApplyConflict(), we form err_detail by calling
> errdetail_apply_conflict(). But when dest is TABLE, we don't use
> err_detail. Shall we skip creating it for dest=TABLE case?
Righ, the above refactoring will take care of this as well
> 4)
> ReportApplyConflict():
> + /*
> + * Get both the conflict log destination and the opened conflict log
> + * relation for insertion.
> + */
> + conflictlogrel = GetConflictLogTableInfo(&dest);
> +
>
> We can move it after errdetail_apply_conflict(), closer to where we
> actually use it.
Same as above
> 5)
> start_apply:
> + /* Open conflict log table and insert the tuple. */
> + conflictlogrel = GetConflictLogTableInfo(&dest);
> + if (ValidateConflictLogTable(conflictlogrel))
> + InsertConflictLogTuple(conflictlogrel);
>
> We can have Assert here too before we call Validate:
> Assert(dest == CONFLICT_LOG_DEST_TABLE || dest == CONFLICT_LOG_DEST_ALL);
Done, I think now we can get rid of ValidateConflictLogTable() as well
because table can not be modified now by user.
> 6)
> start_apply:
> + if (ValidateConflictLogTable(conflictlogrel))
> + InsertConflictLogTuple(conflictlogrel);
> + MyLogicalRepWorker->conflict_log_tuple = NULL;
>
> InsertConflictLogTuple() already sets conflict_log_tuple to NULL.
> Above is not needed.
Make sense.
These fixes will be available in next version.
--
Regards,
Dilip Kumar
Google
