On Thu, 18 Dec 2025 at 14:05, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On Dec 18, 2025, at 03:51, Noah Misch <noah@leadboat.com> wrote:
> >
> > On Wed, Dec 17, 2025 at 10:11:58AM +0530, vignesh C wrote:
> >> The attached v3 version patch has the changes for the same.
> >
> > The "tag" variable needed a change to compensate for the subrinfo->dobj.name
> > change. I plan to push the attached version.
> > <DO_SUBSCRIPTION_REL-v4.patch>
>
> v4 looks solid. A couple of nitpicks:
>
> 1
> ```
> + SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
> + SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
> ```
>
> These two temp pointers can be const, like:
> ```
> const SubRelInfo *srobj1 = *(SubRelInfo *const *) p1;
> const SubRelInfo *srobj2 = *(SubRelInfo *const *) p2;
> ```
I felt the way it is handled in the patch is ok and consistent with
the other variables used in this function.
> 2
> ```
> + /* Sort by subscription name, since (namespace, name) match the rel */
> ```
>
> This comment is correct, but sounds a little insider-ish. Maybe:
>
> /* Tiebreak by subscription name; (namespace, name) already identify the table */
Similarly here too, it is inline with similar comments of other enums
in this function.
Regards,
Vignesh
