RE: row filtering for logical replication

On Tuesday, January 25, 2022 1:55 PM Peter Smith <smithpb2250@gmail.com> wrote:
> 
> A couple more comments for the v69-0001 TAP tests.
> 

Thanks for the comments!

> 
> 1. src/test/subscription/t/027_row_filter.pl
> 
> +# The subscription of the ALL TABLES IN SCHEMA publication means
> there should be
> +# no filtering on the tablesync COPY, so all expect all 5 will be present.
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(x)
> FROM schema_rf_x.tab_rf_x");
> +is($result, qq(5), 'check initial data copy from table tab_rf_x
> should not be filtered');
> +
> +# Similarly, normal filtering after the initial phase will also have
> not effect.
> +# Expected:
> +#     tab_rf_x                       :  5 initial rows + 2 new rows = 7 rows
> +#     tab_rf_partition               :  1 initial row  + 1 new row  = 2 rows
> +$node_publisher->safe_psql('postgres', "INSERT INTO
> schema_rf_x.tab_rf_x (x) VALUES (-99), (99)");
> +$node_publisher->safe_psql('postgres', "INSERT INTO
> schema_rf_x.tab_rf_partitioned (x) VALUES (5), (25)");
> +$node_publisher->wait_for_catchup($appname);
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(x)
> FROM schema_rf_x.tab_rf_x");
> +is($result, qq(7), 'check table tab_rf_x should not be filtered');
> +$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM
> public.tab_rf_partition");
> +is($result, qq(20
> +25), 'check table tab_rf_partition should be filtered');
> 
> That comment ("Similarly, normal filtering after the initial phase will also have
> not effect.") seems no good:
> - it is too vague for the tab_rf_x tablesync
> - it seems completely wrong for the tab_rf_partition table (because that filter is
> working fine)
> 
> I'm not sure exactly what the comment should say, but possibly something like
> this (??):
> 
> BEFORE:
> Similarly, normal filtering after the initial phase will also have not effect.
> AFTER:
> Similarly, the table filter for tab_rf_x (after the initial phase) has no effect when
> combined with the ALL TABLES IN SCHEMA. Meanwhile, the filter for the
> tab_rf_partition does work because that partition belongs to a different
> schema (and publish_via_partition_root = false).
> 

Thanks, I think your change looks good. Changed.

> 
> 2. src/test/subscription/t/027_row_filter.pl
> 
> Here is a 2nd place with the same broken comment:
> 
> +# The subscription of the FOR ALL TABLES publication means there should
> +be no # filtering on the tablesync COPY, so all expect all 5 will be present.
> +my $result = $node_subscriber->safe_psql('postgres', "SELECT count(x)
> FROM tab_rf_x");
> +is($result, qq(5), 'check initial data copy from table tab_rf_x
> should not be filtered');
> +
> +# Similarly, normal filtering after the initial phase will also have
> not effect.
> +# Expected: 5 initial rows + 2 new rows = 7 rows
> +$node_publisher->safe_psql('postgres', "INSERT INTO tab_rf_x (x)
> VALUES (-99), (99)");
> +$node_publisher->wait_for_catchup($appname);
> +$result = $node_subscriber->safe_psql('postgres', "SELECT count(x)
> FROM tab_rf_x");
> +is($result, qq(7), 'check table tab_rf_x should not be filtered');
> 
> Here I also think the comment maybe should just say something like:
> 
> BEFORE:
> Similarly, normal filtering after the initial phase will also have not effect.
> AFTER:
> Similarly, the table filter for tab_rf_x (after the initial phase) has no effect when
> combined with the ALL TABLES IN SCHEMA.

Changed.

Attach the V71 patch set which addressed the above comments.
The patch also includes the changes:
- Changed the function RelationBuildPublicationDesc's signature to be void and
  pass the PublicationDesc* from stack instead of palloc-ing it. [1]
- Removed the Push/Pop ActiveSnapshot related code. IIRC, these functions are
  needed when we execute functions which will execute SQL(via SPI functions) to
  access the database. I think we don't need the ActiveSnapshot for now as we
  only support built-in immutable in the row filter which should only use the
  argument values passed to the function.
- Adjusted some comments in pgoutput.c.

[1] https://www.postgresql.org/message-id/CAHut%2BPsRTtXoYQiRqxwvyrcmkDMm-kR4GkvD9-nAqNrk4A3aCQ%40mail.gmail.com

Best regards,
Hou zj