On Wed, Jan 5, 2022 at 1:05 PM wangw.f...@fujitsu.com <wangw.f...@fujitsu.com> wrote: > > On Thu, Jan 4, 2022 at 00:54 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Modified in v58 [1] as suggested > Thanks for updating the patches. > A few comments about v58-0001 and v58-0002. > > v58-0001 > 1. > How about modifying the following loop in copy_table by using for_each_from > instead of foreach? > Like the invocation of for_each_from in function get_rule_expr. > from: > if (qual != NIL) > { > ListCell *lc; > bool first = true; > > appendStringInfoString(&cmd, " WHERE "); > foreach(lc, qual) > { > char *q = strVal(lfirst(lc)); > > if (first) > first = false; > else > appendStringInfoString(&cmd, " OR "); > appendStringInfoString(&cmd, q); > } > list_free_deep(qual); > } > change to: > if (qual != NIL) > { > ListCell *lc; > char *q = strVal(linitial(qual)); > > appendStringInfo(&cmd, " WHERE %s", q); > for_each_from(lc, qual, 1) > { > q = strVal(lfirst(lc)); > appendStringInfo(&cmd, " OR %s", q); > } > list_free_deep(qual); > } > > 2. > I find the API of get_rel_sync_entry is modified. > -get_rel_sync_entry(PGOutputData *data, Oid relid) > +get_rel_sync_entry(PGOutputData *data, Relation relation) > It looks like just moving the invocation of RelationGetRelid from outside into > function get_rel_sync_entry. I am not sure whether this modification is > necessary to this feature or not. > > v58-0002 > 1. > In function pgoutput_row_filter_init, if no_filter is set, I think we do not > need to add row filter to list(rfnodes). > So how about changing three conditions when add row filter to rfnodes like > this: > - if (pub->pubactions.pubinsert) > + if (pub->pubactions.pubinsert && > !no_filter[idx_ins]) > { > rfnode = > stringToNode(TextDatumGetCString(rfdatum)); > rfnodes[idx_ins] = > lappend(rfnodes[idx_ins], rfnode); > } >
I think currently there is no harm done with the current code because even there was no_filter[xxx] then any gathered rfnodes[xxx] will be later cleaned up and ignored anyway, so this change is not really necessary. OTOH your suggestion could be a tiny bit more efficient for some cases if there are many publications. so LGTM. ------ Kind Regards, Peter Smith. Fujitsu Australia.