Re: Proposal: Filter irrelevant change before reassemble transactions during logical decoding - Mailing list pgsql-hackers

Hi Ajin,

Some review comments for patch v16-0003.

======
Patch

1.
mode change 100644 => 100755 src/test/subscription/t/001_rep_changes.pl

What's this mode change for?

======
Commit message

2. missing commit message

======
src/backend/replication/logical/reorderbuffer.c

3.
-#define CHANGES_THRESHOLD_FOR_FILTER 100
+#define CHANGES_THRESHOLD_FOR_FILTER 0

Hmm. You cannot leave a thing like this in the patch because it seems
like just a temporary hack and means the patch cannot be committed in
this form. It needs some kind of more permanent testing solution,
allowing the tests of this patch can executed efficiently, and the
patch pushed, all without impacting the end-user. Maybe consider if
it's possible to have some injection point so the text can "inject" a
zero here (??).

======
src/test/subscription/t/001_rep_changes.pl

4.
# Create new tables on publisher and subscriber
$node_publisher->safe_psql('postgres', "CREATE TABLE pub_table (id int
primary key, data text)");
$node_publisher->safe_psql('postgres', "CREATE TABLE unpub_table (id
int primary key, data text)");
$node_publisher->safe_psql('postgres', "CREATE TABLE insert_only_table
(id int primary key, data text)");
$node_publisher->safe_psql('postgres', "CREATE TABLE delete_only_table
(id int primary key, data text)");

You could combine all those DDLs.

~~~

5.
$node_subscriber->safe_psql('postgres', "CREATE TABLE pub_table (id
int primary key, data text)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE unpub_table (id
int primary key, data text)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE
insert_only_table (id int primary key, data text)");
$node_subscriber->safe_psql('postgres', "CREATE TABLE
delete_only_table (id int primary key, data text)");

5a.
You could combine all those DDLs.

~

5b.
There are double blank lines here.

~~~

6.
# Setup logical replication publications
$node_publisher->safe_psql('postgres', "CREATE PUBLICATION pub_all FOR
TABLE pub_table");
$node_publisher->safe_psql('postgres', "CREATE PUBLICATION
pub_insert_only FOR TABLE insert_only_table WITH (publish = insert)");
$node_publisher->safe_psql('postgres', "CREATE PUBLICATION
pub_delete_only FOR TABLE delete_only_table WITH (publish = delete)");

You could combine all those DDLs.

~~~

7.
# Setup logical replication subscription
$node_subscriber->safe_psql('postgres', "CREATE SUBSCRIPTION sub_all
CONNECTION '$publisher_connstr' PUBLICATION pub_all");
$node_subscriber->safe_psql('postgres', "CREATE SUBSCRIPTION
sub_insert_only CONNECTION '$publisher_connstr' PUBLICATION
pub_insert_only");
$node_subscriber->safe_psql('postgres', "CREATE SUBSCRIPTION
sub_delete_only CONNECTION '$publisher_connstr' PUBLICATION
pub_delete_only");


7a.
comment should be plural /subscription/subscriptions/

~

7b.
You could combine all those DDL.

~~~

8.
# Insert, delete, and update tests for restricted publication tables
$log_location = -s $node_publisher->logfile;
$node_publisher->safe_psql('postgres', "INSERT INTO insert_only_table
VALUES (1, 'to be inserted')");
$node_publisher->safe_psql('postgres', "UPDATE insert_only_table SET
data = 'updated' WHERE id = 1");
$logfile = slurp_file($node_publisher->logfile, $log_location);
ok($logfile =~ qr/Filtering UPDATE/,
'unpublished UPDATE is filtered');

$log_location = -s $node_publisher->logfile;
$node_publisher->safe_psql('postgres', "DELETE FROM insert_only_table
WHERE id = 1");
$logfile = slurp_file($node_publisher->logfile, $log_location);
ok($logfile =~ qr/Filtering DELETE/,
'unpublished DELETE is filtered');

$log_location = -s $node_publisher->logfile;
$node_publisher->safe_psql('postgres', "INSERT INTO delete_only_table
VALUES (1, 'to be deleted')");
$logfile = slurp_file($node_publisher->logfile, $log_location);
ok($logfile =~ qr/Filtering INSERT/,
'unpublished INSERT is filtered');

~
8a.
Maybe it is clearer to say "...for publications with restricted pubactions"

~

8b.
Change the comment or rearrange the tests so they are in the same
order as described

~

8c.
Looking at the expected logs I wondered if it might be nicer for the
pgoutput_filter_change doing this logging to also emit the relation
name.

~~~

9.
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_all");
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_insert_only");
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_delete_only");
+

9a.
You could combine all those DDL

~

9b.
Add some simple comment like "# cleanup"

~

9c.
Any reason why you dropped the subscriptions but not the publications?

======
Kind Regards,
Peter Smith.
Fujitsu Australia