On Mon, Jan 17, 2022 at 9:00 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> wrote: > > On Mon, Jan 17, 2022 12:34 PM Peter Smith <smithpb2...@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.
