Hi Jeff,
On Tue, Jan 16, 2024 at 7:25 AM Jeff Davis <pg...@j-davis.com> wrote:
>
> On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> > I think 0004 needs a bit more work, so I'm leaving it off for now,
> > but
> > I'll bring it back in the next patch set.
>
> Here's the next patch set. 0001 - 0003 are mostly the same with some
> improved error messages and some code fixes. I am looking to start
> committing 0001 - 0003 soon, as they have received some feedback
> already and 0004 isn't required for the earlier patches to be useful.
>
I am reviewing the patches. Here are some random comments.
0002 adds a prefix "regress_" to almost every object that is created
in foreign_data.sql. The commit message doesn't say why it's doing so.
But more importantly, the new tests added are lost in all the other
changes. It will be good to have prefix adding changes into its own
patch explaining the reason. The new tests may stay in 0002.
Interestingly the foreign server created in the new tests doesn't have
"regress_" prefix. Why?
Dummy FDW makes me nervous. The way it's written, it may grow into a
full-fledged postgres_fdw and in the process might acquire the same
concerns that postgres_fdw has today. But I will study the patches and
discussion around it more carefully.
I enhanced the postgres_fdw TAP test to use foreign table. Please see
the attached patch. It works as expected. Of course a follow-on work
will require linking the local table and its replica on the publisher
table so that push down will work on replicated tables. But the
concept at least works with your changes. Thanks for that.
I am not sure we need a full-fledged TAP test for testing
subscription. I wouldn't object to it, but TAP tests are heavy. It
should be possible to write the same test as a SQL test by creating
two databases and switching between them. Do you think it's worth
trying that way?
> 0004 could use more discussion. The purpose is to split the privileges
> of pg_create_subscription into two: pg_create_subscription, and
> pg_create_connection. By separating the privileges, it's possible to
> allow someone to create/manage subscriptions to a predefined set of
> foreign servers (on which they have USAGE privileges) without allowing
> them to write an arbitrary connection string.
Haven't studied this patch yet. Will continue reviewing the patches.
--
Best Wishes,
Ashutosh Bapat
diff --git a/contrib/postgres_fdw/t/010_subscription.pl
b/contrib/postgres_fdw/t/010_subscription.pl
index d1d80d0679..3ae2b6da4a 100644
--- a/contrib/postgres_fdw/t/010_subscription.pl
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -20,7 +20,7 @@ $node_subscriber->start;
# Create some preexisting content on publisher
$node_publisher->safe_psql('postgres',
- "CREATE TABLE tab_ins AS SELECT generate_series(1,1002) AS a");
+ "CREATE TABLE tab_ins AS SELECT a, a + 1 as b FROM
generate_series(1,1002) AS a");
# Replicate the changes without columns
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_no_col()");
@@ -29,7 +29,7 @@ $node_publisher->safe_psql('postgres',
# Setup structure on subscriber
$node_subscriber->safe_psql('postgres', "CREATE EXTENSION postgres_fdw");
-$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_ins (a int, b int)");
# Setup logical replication
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -45,6 +45,9 @@ $node_subscriber->safe_psql('postgres',
"CREATE USER MAPPING FOR PUBLIC SERVER tap_server"
);
+$node_subscriber->safe_psql('postgres',
+ "CREATE FOREIGN TABLE f_tab_ins (a int, b int) SERVER tap_server
OPTIONS(table_name 'tab_ins')"
+);
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH
(password_required=false)"
);
@@ -53,16 +56,16 @@ $node_subscriber->safe_psql('postgres',
$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
my $result =
- $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_ins");
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM (SELECT f.b =
l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a = f.a) WHERE match");
is($result, qq(1002), 'check initial data was copied to subscriber');
$node_publisher->safe_psql('postgres',
- "INSERT INTO tab_ins SELECT generate_series(1,50)");
+ "INSERT INTO tab_ins SELECT a, a + 1 FROM generate_series(1003,1050)
a");
$node_publisher->wait_for_catchup('tap_sub');
$result =
- $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM tab_ins");
-is($result, qq(1052), 'check initial data was copied to subscriber');
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM (SELECT f.b =
l.b as match FROM tab_ins l, f_tab_ins f WHERE l.a = f.a) WHERE match");
+is($result, qq(1050), 'check initial data was copied to subscriber');
done_testing();
