On Mon, Jan 17, 2022 at 9:00 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Mon, Jan 17, 2022 12:34 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Here are some review comments for v65-0001 (review of updates since
> > v64-0001)
>
> Thanks for the comments!
>
> > ~~~
> >
> > 1. src/include/commands/publicationcmds.h - rename func
> >
> > +extern bool contain_invalid_rfcolumn(Oid pubid, Relation relation,
> > +List *ancestors, AttrNumber *invalid_rfcolumn);
> >
> > I thought that function should be called "contains_..." instead of "contain_...".
> >
> > ~~~
> >
> > 2. src/backend/commands/publicationcmds.c - rename funcs
> >
> > Suggested renaming (same as above #1).
> >
> > "contain_invalid_rfcolumn_walker" --> "contains_invalid_rfcolumn_walker"
> > "contain_invalid_rfcolumn" --> "contains_invalid_rfcolumn"
> >
> > Also, update it in the comment for rf_context:
> > +/*
> > + * Information used to validate the columns in the row filter
> > +expression. See
> > + * contain_invalid_rfcolumn_walker for details.
> > + */
>
> I am not sure about the name because many existing
> functions are named contain_xxx_xxx.
> (for example contain_mutable_functions)
>
I also see many similar functions whose name start with contain_* like
contain_var_clause, contain_agg_clause, contain_window_function, etc.
So, it is probably okay to retain the name as it is in the patch.
--
With Regards,
Amit Kapila.