Re: row filtering for logical replication

On Wed, Jan 5, 2022 at 1:05 PM wangw(dot)fnst(at)fujitsu(dot)com
<wangw(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Thu, Jan 4, 2022 at 00:54 PM Peter Smith <smithpb2250(at)gmail(dot)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.