On Fri, Jul 29, 2022 at 8:26 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> On Fri, Jul 29, 2022 at 11:55 AM houzj.f...@fujitsu.com
> <houzj.f...@fujitsu.com> wrote:
> >
> > On Friday, July 29, 2022 7:17 AM Peter Smith <smithpb2...@gmail.com> wrote:
> > > During a recent review, I happened to notice that in the file
> > > src/backend/catalog/pg_publication.c the two functions
> > > 'is_publishable_class'
> > > and 'is_publishable_relation' used to be [1] adjacent in the source code.
> > > This is
> > > also evident in 'is_publishable_relation' because the wording of the
> > > function
> > > comment just refers to the prior function (e.g. "Another variant of this,
> > > taking a
> > > Relation.") and also this just "wraps" the prior function.
> > >
> > > It seems that sometime last year another commit [2] inadvertently inserted
> > > another function ('filter_partitions') between those aforementioned, and
> > > that
> > > means the "Another variant of this" comment doesn't make much sense
> > > anymore.
> >
> > Agreed.
> >
> > Personally, I think it would be better to modify the comments of
> > is_publishable_relation and directly mention the function name it refers to
> > which can prevent future code to break it again.
>
> I'd intended only to make the minimal changes necessary to set things
> right again, but your way is better.
>
Yeah, Hou-San's suggestion sounds better to me as well.
> >
> > Besides,
> >
> > /*
> > * Returns if relation represented by oid and Form_pg_class entry
> > * is publishable.
> > *
> > * Does same checks as the above,
> >
> > This comment was also intended to refer to the function
> > check_publication_add_relation(), but is invalid now because there is
> > another
> > function check_publication_add_schema() inserted between them. We'd better
> > fix
> > this as well.
>
+1. Here, I think it will be better to add the function name in the
comments and keep the current order as it is.
--
With Regards,
Amit Kapila.
