On Tuesday, August 13, 2024 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 13, 2024 at 10:09 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Here is V13 patch set which addressed above comments.
> >
>
> 1.
> +ReportApplyConflict(int elevel, ConflictType type, EState *estate,
> +ResultRelInfo *relinfo,
>
> The change looks better but it would still be better to keep elevel and type after
> relinfo. The same applies to other places as well.
Changed.
>
> 2.
> + * The caller should ensure that the index with the OID 'indexoid' is locked.
> + *
> + * Refer to errdetail_apply_conflict for the content that will be
> +included in
> + * the DETAIL line.
> + */
> +void
> +ReportApplyConflict(int elevel, ConflictType type, EState *estate,
>
> Is it possible to add an assert to ensure that the index is locked by the caller?
Added.
>
> 3.
> +static char *
> +build_tuple_value_details(EState *estate, ResultRelInfo *relinfo,
> + TupleTableSlot *searchslot,
> + TupleTableSlot *localslot,
> + TupleTableSlot *remoteslot,
> + Oid indexoid)
> {
> ...
> ...
> + /*
> + * If 'searchslot' is NULL and 'indexoid' is valid, it indicates that
> + we
> + * are reporting the unique constraint violation conflict, in which
> + case
> + * the conflicting key values will be reported.
> + */
> + if (OidIsValid(indexoid) && !searchslot) {
> ...
> ...
> }
>
> This indirect way of inferencing constraint violation looks fragile.
> The caller should pass the required information explicitly and then you can
> have the required assertions here.
>
> Apart from the above, I have made quite a few changes in the code comments
> and LOG messages in the attached.
Thanks. I have addressed above comments and merged the changes.
Here is the V14 patch.
Best Regards,
Hou zj
