From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Peter Smith <smithpb2250(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, Euler Taveira <euler(at)eulerto(dot)com>, Greg Nancarrow <gregn4422(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Önder Kalacı <onderkalaci(at)gmail(dot)com>, japin <japinli(at)hotmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, David Steele <david(at)pgmasters(dot)net>, Craig Ringer <craig(at)2ndquadrant(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: row filtering for logical replication |
Date: | 2022-01-17 15:28:17 |
Message-ID: | OS0PR01MB5716F698E4C2476B98AF2B8D94579@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v66-0001-Allow-specifying-row-filter-for-logical-replication-.patch | application/octet-stream | 148.3 KB |
v66-0002-Row-filter-tab-auto-complete-and-pgdump.patch | application/octet-stream | 5.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2022-01-17 15:30:00 | RE: row filtering for logical replication |
Previous Message | Bharath Rupireddy | 2022-01-17 15:25:14 | Re: Blank archive_command |