RE: row filtering for logical replication

On Mon, Jan 17, 2022 7:23 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jan 17, 2022 at 3:19 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > Some other comments:
> > ==================
> >
>
> Few more comments:
> ==================
> 1.
> +pgoutput_row_filter_init_expr(Node *rfnode) { ExprState *exprstate;
> + Expr *expr;
> +
> + /*
> + * This is the same code as ExecPrepareExpr() but that is not used
> + because
> + * we have no EState to pass it.
>
> Isn't it better to say "This is the same code as ExecPrepareExpr() but that is not
> used because we want to cache the expression"? I feel if we want we can allocate
> Estate as the patch is doing in pgoutput_row_filter(), no?

Changed as suggested.

> 2.
> + elog(DEBUG3, "row filter evaluates to %s (isnull: %s)",
> + DatumGetBool(ret) ? "true" : "false",
> + isnull ? "true" : "false");
> +
> + if (isnull)
> + return false;
>
> Won't the isnull condition's result in elog should be reversed?

Changed.

> 3.
> + /*
> + * If the publication is FOR ALL TABLES IN SCHEMA and it overlaps
> + * with the current relation in the same schema then this is also
> + * treated same as if this table has no row filters (even if for
> + * other publications it does).
> + */
> + else if (list_member_oid(schemaPubids, pub->oid)) pub_no_filter =
> + true;
>
> The code will appear better if you can move the comments inside else if. There
> are other places nearby this comment where we can follow the same style.

Changed.

> 4.
> + * Multiple ExprState entries might be used if there are multiple
> + * publications for a single table. Different publication actions don't
> + * allow multiple expressions to always be combined into one, so there
> + is
> + * one ExprState per publication action. The exprstate array is indexed
> + by
> + * ReorderBufferChangeType.
> + */
> + bool exprstate_valid;
> +
> + /* ExprState array for row filter. One per publication action. */
> + ExprState *exprstate[NUM_ROWFILTER_PUBACTIONS];
>
> It is not clear from comments here or at other places as to why we need an array
> for row filter expressions? Can you please add comments to explain the same?
> IIRC, it is primarily due to the reason that we don't want to add the restriction
> that the publish operation 'insert'
> should also honor RI columns restriction. If there are other reasons then let's add
> those to comments as well.
I will think over this and update in next version.

Attach the V66 patch set which addressed Amit, Peter and Greg's comments.

Best regards,
Hou zj