Re: row filtering for logical replication

On Thu, Dec 2, 2021 at 9:29 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thur, Dec 2, 2021 5:21 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > PSA the v44* set of patches.
> >
> > The following review comments are addressed:
> >
> > v44-0001 main patch
> > - Renamed the TAP test 026->027 due to clash caused by recent commit [1]
> > - Refactored table_close [Houz 23/11] #2
> > - Alter compare where clauses [Amit 24/11] #0
> > - PG docs CREATE SUBSCRIPTION [Tang 30/11] #2
> > - PG docs CREATE PUBLICATION [Vignesh 30/11] #1, #4, [Tang 30/11] #1, [Tomas
> > 23/9] #2
> >
> > v44-0002 validation walker
> > - Add NullTest support [Peter 18/11]
> > - Update comments [Amit 24/11] #3
> > - Disallow user-defined types [Amit 24/11] #4
> > - Errmsg - skipped because handled by top-up [Vignesh 23/11] #2
> > - Removed #if 0 [Vignesh 30/11] #2
> >
> > v44-0003 new/old tuple
> > - NA
> >
> > v44-0004 tab-complete and pgdump
> > - Handle table-list commas better [Vignesh 23/11] #2
> >
> > v44-0005 top-up patch for validation
> > - (This patch will be added again later)
>
> Attach the v44-0005 top-up patch.

Thanks for the updated patch, few comments:
1) Both testpub5a and testpub5c publication are same, one of them can be removed
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub5a FOR TABLE testpub_rf_tbl1 WHERE (a > 1)
WITH (publish="insert");
+CREATE PUBLICATION testpub5b FOR TABLE testpub_rf_tbl1;
+CREATE PUBLICATION testpub5c FOR TABLE testpub_rf_tbl1 WHERE (a > 3)
WITH (publish="insert");
+RESET client_min_messages;
+\d+ testpub_rf_tbl1
+DROP PUBLICATION testpub5a, testpub5b, testpub5c;

testpub5b will be covered in the earlier existing case above:
ALTER PUBLICATION testpib_ins_trunct ADD TABLE pub_test.testpub_nopk,
testpub_tbl1;

\d+ pub_test.testpub_nopk
\d+ testpub_tbl1

I felt test related to testpub5b is also not required

2) testpub5 and testpub_syntax2 are similar, one of them can be removed:
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub5 FOR TABLE testpub_rf_tbl1,
testpub_rf_tbl2 WHERE (c <> 'test' AND d < 5);
+RESET client_min_messages;
+\dRp+ testpub5

+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub_syntax2 FOR TABLE testpub_rf_tbl1,
testpub_rf_myschema.testpub_rf_tbl5 WHERE (h < 999);
+RESET client_min_messages;
+\dRp+ testpub_syntax2
+DROP PUBLICATION testpub_syntax2;

3) testpub7 can be renamed to testpub6 to maintain the continuity
since the previous testpub6 did not succeed:
+CREATE OPERATOR =#> (PROCEDURE = testpub_rf_func, LEFTARG = integer,
RIGHTARG = integer);
+CREATE PUBLICATION testpub6 FOR TABLE testpub_rf_tbl3 WHERE (e =#> 27);
+-- fail - WHERE not allowed in DROP
+ALTER PUBLICATION testpub5 DROP TABLE testpub_rf_tbl3 WHERE (e < 27);
+-- fail - cannot ALTER SET table which is a member of a pre-existing schema
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub7 FOR ALL TABLES IN SCHEMA testpub_rf_myschema1;
+ALTER PUBLICATION testpub7 SET ALL TABLES IN SCHEMA
testpub_rf_myschema1, TABLE testpub_rf_myschema1.testpub_rf_tb16;
+RESET client_min_messages;

4) Did this test intend to include where clause in testpub_rf_tb16, if
so it can be added:
+-- fail - cannot ALTER SET table which is a member of a pre-existing schema
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub7 FOR ALL TABLES IN SCHEMA testpub_rf_myschema1;
+ALTER PUBLICATION testpub7 SET ALL TABLES IN SCHEMA
testpub_rf_myschema1, TABLE testpub_rf_myschema1.testpub_rf_tb16;
+RESET client_min_messages;

5) It should be removed from typedefs.list too:
-/* For rowfilter_walker. */
-typedef struct {
-       Relation        rel;
-       bool            check_replident; /* check if Var is
bms_replident member? */
-       Bitmapset  *bms_replident;
-} rf_context;
-

Regards,
Vignesh