More cleanup of a2ab9c0.

Require SELECT privileges when performing UPDATE or DELETE, to be
consistent with the way a normal UPDATE or DELETE command works.

Simplify subscription test it so that it runs faster. Also, wait for
initial table sync to complete to avoid intermittent failures.

Minor doc fixup.

Discussion: https://postgr.es/m/CAA4eK1L3-qAtLO4sNGaNhzcyRi_Ufmh2YPPnUjkROBK0tN%3Dx%3Dg%40mail.gmail.com
Discussion: https://postgr.es/m/1514479.1641664638%40sss.pgh.pa.us
Discussion: https://postgr.es/m/Ydkfj5IsZg7mQR0g@paquier.xyz

Author: jeff-davis

doc/src/sgml/logical-replication.sgml

@@ -336,7 +336,7 @@
    cause replication conflicts, as will enabled
    <link linkend="ddl-rowsecurity">row-level security</link> on target tables
    that the subscription owner is subject to, without regard to whether any
-   policy would ordinary reject the <command>INSERT</command>,
+   policy would ordinarily reject the <command>INSERT</command>,
    <command>UPDATE</command>, <command>DELETE</command> or
    <command>TRUNCATE</command> which is being replicated.  This restriction on
    row-level security may be lifted in a future version of

src/backend/replication/logical/worker.c

@@ -1991,6 +1991,12 @@ FindReplTupleInLocalRel(EState *estate, Relation localrel,
 	Oid			idxoid;
 	bool		found;
 
+	/*
+	 * Regardless of the top-level operation, we're performing a read here, so
+	 * check for SELECT privileges.
+	 */
+	TargetPrivilegesCheck(localrel, ACL_SELECT);
+
 	*localslot = table_slot_create(localrel, &estate->es_tupleTable);
 
 	idxoid = GetRelationIdentityOrPK(localrel);

src/test/subscription/t/027_nosuperuser.pl

@@ -5,7 +5,7 @@
 use strict;
 use warnings;
 use PostgreSQL::Test::Cluster;
-use Test::More tests => 100;
+use Test::More tests => 14;
 
 my ($node_publisher, $node_subscriber, $publisher_connstr, $result, $offset);
 $offset = 0;
@@ -95,19 +95,6 @@ sub grant_bypassrls
 $node_publisher->start;
 $node_subscriber->start;
 $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
-my %range_a = (
-  publisher => 'FROM (0) TO (15)',
-  subscriber => 'FROM (0) TO (5)');
-my %range_b = (
-  publisher => 'FROM (15) TO (30)',
-  subscriber => 'FROM (5) TO (30)');
-my %list_a = (
-  publisher => 'IN (1, 3, 5, 7, 9, 11, 13, 15, 17, 19, 21, 23, 25, 27, 29)',
-  subscriber => 'IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16)',
-);
-my %list_b = (
-  publisher => 'IN (2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30)',
-  subscriber => 'IN (17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30)');
 my %remainder_a = (
   publisher => 0,
   subscriber => 1);
@@ -117,10 +104,6 @@ sub grant_bypassrls
 
 for my $node ($node_publisher, $node_subscriber)
 {
-  my $range_a = $range_a{$node->name};
-  my $range_b = $range_b{$node->name};
-  my $list_a = $list_a{$node->name};
-  my $list_b = $list_b{$node->name};
   my $remainder_a = $remainder_a{$node->name};
   my $remainder_b = $remainder_b{$node->name};
   $node->safe_psql('postgres', qq(
@@ -135,26 +118,6 @@ sub grant_bypassrls
   ALTER TABLE alice.unpartitioned REPLICA IDENTITY FULL;
   GRANT SELECT ON TABLE alice.unpartitioned TO regress_admin;
 
-  CREATE TABLE alice.rangepart (i INTEGER) PARTITION BY RANGE (i);
-  ALTER TABLE alice.rangepart REPLICA IDENTITY FULL;
-  GRANT SELECT ON TABLE alice.rangepart TO regress_admin;
-  CREATE TABLE alice.rangepart_a PARTITION OF alice.rangepart
-    FOR VALUES $range_a;
-  ALTER TABLE alice.rangepart_a REPLICA IDENTITY FULL;
-  CREATE TABLE alice.rangepart_b PARTITION OF alice.rangepart
-    FOR VALUES $range_b;
-  ALTER TABLE alice.rangepart_b REPLICA IDENTITY FULL;
-
-  CREATE TABLE alice.listpart (i INTEGER) PARTITION BY LIST (i);
-  ALTER TABLE alice.listpart REPLICA IDENTITY FULL;
-  GRANT SELECT ON TABLE alice.listpart TO regress_admin;
-  CREATE TABLE alice.listpart_a PARTITION OF alice.listpart
-    FOR VALUES $list_a;
-  ALTER TABLE alice.listpart_a REPLICA IDENTITY FULL;
-  CREATE TABLE alice.listpart_b PARTITION OF alice.listpart
-    FOR VALUES $list_b;
-  ALTER TABLE alice.listpart_b REPLICA IDENTITY FULL;
-
   CREATE TABLE alice.hashpart (i INTEGER) PARTITION BY HASH (i);
   ALTER TABLE alice.hashpart REPLICA IDENTITY FULL;
   GRANT SELECT ON TABLE alice.hashpart TO regress_admin;
@@ -170,171 +133,133 @@ sub grant_bypassrls
 SET SESSION AUTHORIZATION regress_alice;
 
 CREATE PUBLICATION alice
-  FOR TABLE alice.unpartitioned, alice.rangepart, alice.listpart, alice.hashpart
+  FOR TABLE alice.unpartitioned, alice.hashpart
   WITH (publish_via_partition_root = true);
 ));
 $node_subscriber->safe_psql('postgres', qq(
 SET SESSION AUTHORIZATION regress_admin;
 CREATE SUBSCRIPTION admin_sub CONNECTION '$publisher_connstr' PUBLICATION alice;
 ));
 
+$node_publisher->wait_for_catchup('admin_sub');
+
+# Wait for initial sync to finish as well
+my $synced_query =
+  "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('s', 'r');";
+$node_subscriber->poll_query_until('postgres', $synced_query)
+  or die "Timed out while waiting for subscriber to synchronize data";
+
 # Verify that "regress_admin" can replicate into the tables
 #
-my @tbl = (qw(unpartitioned rangepart listpart hashpart));
-for my $tbl (@tbl)
-{
-  publish_insert("alice.$tbl", 1);
-  publish_insert("alice.$tbl", 3);
-  publish_insert("alice.$tbl", 5);
-  expect_replication(
-    "alice.$tbl", 3, 1, 5,
-    "superuser admin replicates insert into $tbl");
-  publish_update("alice.$tbl", 1 => 7);
-  expect_replication(
-    "alice.$tbl", 3, 3, 7,
-    "superuser admin replicates update into $tbl");
-  publish_delete("alice.$tbl", 3);
-  expect_replication(
-    "alice.$tbl", 2, 5, 7,
-    "superuser admin replicates delete into $tbl");
-}
-
-# Repeatedly revoke and restore superuser privilege for "regress_admin", verifying
-# that replication fails while superuser privilege is missing, but works again and
-# catches up once superuser is restored.
+publish_insert("alice.unpartitioned", 1);
+publish_insert("alice.unpartitioned", 3);
+publish_insert("alice.unpartitioned", 5);
+publish_update("alice.unpartitioned", 1 => 7);
+publish_delete("alice.unpartitioned", 3);
+expect_replication(
+    "alice.unpartitioned", 2, 5, 7,
+    "superuser admin replicates into unpartitioned");
+
+# Revoke and restore superuser privilege for "regress_admin",
+# verifying that replication fails while superuser privilege is
+# missing, but works again and catches up once superuser is restored.
 #
-for my $tbl (@tbl)
-{
-  revoke_superuser("regress_admin");
-  publish_insert("alice.$tbl", 3);
-  expect_failure("alice.$tbl", 2, 5, 7,
-    qr/ERROR:  permission denied for table $tbl/msi,
-    "non-superuser admin fails to replicate insert");
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 3, 3, 7,
-    "admin with restored superuser privilege replicates insert");
-
-  revoke_superuser("regress_admin");
-  publish_update("alice.$tbl", 3 => 9);
-  expect_failure("alice.$tbl", 3, 3, 7,
-    qr/ERROR:  permission denied for table $tbl/msi,
-    "non-superuser admin fails to replicate update");
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 3, 5, 9,
-    "admin with restored superuser privilege replicates update");
-
-  revoke_superuser("regress_admin");
-  publish_delete("alice.$tbl", 5);
-  expect_failure("alice.$tbl", 3, 5, 9,
-    qr/ERROR:  permission denied for table $tbl/msi,
-    "non-superuser admin fails to replicate delete");
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 2, 7, 9,
-    "admin with restored superuser privilege replicates delete");
-}
-
-# Grant privileges on the target tables to "regress_admin" so that superuser
-# privileges are not necessary for replication.
+revoke_superuser("regress_admin");
+publish_update("alice.unpartitioned", 5 => 9);
+expect_failure("alice.unpartitioned", 2, 5, 7,
+			   qr/ERROR:  permission denied for table unpartitioned/msi,
+			   "non-superuser admin fails to replicate update");
+grant_superuser("regress_admin");
+expect_replication("alice.unpartitioned", 2, 7, 9,
+				   "admin with restored superuser privilege replicates update");
+
+# Grant INSERT, UPDATE, DELETE privileges on the target tables to
+# "regress_admin" so that superuser privileges are not necessary for
+# replication.
+#
+# Note that UPDATE and DELETE also require SELECT privileges, which
+# will be granted in subsequent test.
 #
 $node_subscriber->safe_psql('postgres', qq(
 ALTER ROLE regress_admin NOSUPERUSER;
 SET SESSION AUTHORIZATION regress_alice;
-GRANT ALL PRIVILEGES ON
+GRANT INSERT,UPDATE,DELETE ON
   alice.unpartitioned,
-  alice.rangepart, alice.rangepart_a, alice.rangepart_b,
-  alice.listpart, alice.listpart_a, alice.listpart_b,
   alice.hashpart, alice.hashpart_a, alice.hashpart_b
   TO regress_admin;
+REVOKE SELECT ON alice.unpartitioned FROM regress_admin;
 ));
-for my $tbl (@tbl)
-{
-  publish_insert("alice.$tbl", 11);
-  publish_update("alice.$tbl", 7 => 13);
-  publish_delete("alice.$tbl", 9);
-  expect_replication("alice.$tbl", 2, 11, 13,
-    "nosuperuser admin with all table privileges can replicate into $tbl");
-}
 
-# Enable RLS on the target tables and check that "regress_admin" can only
-# replicate into them when superuser.  Note that RLS must be enabled on the
-# partitions, not the partitioned tables, since the partitions are the targets
-# of the replication.
+publish_insert("alice.unpartitioned", 11);
+expect_replication("alice.unpartitioned", 3, 7, 11,
+				   "nosuperuser admin with INSERT privileges can replicate into unpartitioned");
+
+publish_update("alice.unpartitioned", 7 => 13);
+expect_failure("alice.unpartitioned", 3, 7, 11,
+			   qr/ERROR:  permission denied for table unpartitioned/msi,
+			   "non-superuser admin without SELECT privileges fails to replicate update");
+
+# Now grant SELECT
 #
 $node_subscriber->safe_psql('postgres', qq(
 SET SESSION AUTHORIZATION regress_alice;
-ALTER TABLE alice.unpartitioned ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.rangepart_a ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.rangepart_b ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.listpart_a ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.listpart_b ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.hashpart_a ENABLE ROW LEVEL SECURITY;
-ALTER TABLE alice.hashpart_b ENABLE ROW LEVEL SECURITY;
+GRANT SELECT ON
+  alice.unpartitioned,
+  alice.hashpart, alice.hashpart_a, alice.hashpart_b
+  TO regress_admin;
 ));
-for my $tbl (@tbl)
-{
-  revoke_superuser("regress_admin");
-  publish_insert("alice.$tbl", 15);
-  expect_failure("alice.$tbl", 2, 11, 13,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "non-superuser admin fails to replicate insert into rls enabled table");
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 3, 11, 15,
-    "admin with restored superuser privilege replicates insert into rls enabled $tbl");
-
-  revoke_superuser("regress_admin");
-  publish_update("alice.$tbl", 11 => 17);
-  expect_failure("alice.$tbl", 3, 11, 15,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "non-superuser admin fails to replicate update into rls enabled $tbl");
-
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 3, 13, 17,
-    "admin with restored superuser privilege replicates update into rls enabled $tbl");
-
-  revoke_superuser("regress_admin");
-  publish_delete("alice.$tbl", 13);
-  expect_failure("alice.$tbl", 3, 13, 17,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "non-superuser admin fails to replicate delete into rls enabled $tbl");
-  grant_superuser("regress_admin");
-  expect_replication("alice.$tbl", 2, 15, 17,
-    "admin with restored superuser privilege replicates delete into rls enabled $tbl");
-}
 
-# Revoke superuser from "regress_admin".  Check that the admin can now only
-# replicate into alice's table when admin has the bypassrls privilege.
+publish_delete("alice.unpartitioned", 9);
+expect_replication("alice.unpartitioned", 2, 11, 13,
+				   "nosuperuser admin with all table privileges can replicate into unpartitioned");
+
+# Test partitioning
 #
-for my $tbl (@tbl)
-{
-  revoke_superuser("regress_admin");
-  revoke_bypassrls("regress_admin");
-  publish_insert("alice.$tbl", 19);
-  expect_failure("alice.$tbl", 2, 15, 17,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "nobypassrls admin fails to replicate insert into rls enabled $tbl");
-  grant_bypassrls("regress_admin");
-  expect_replication("alice.$tbl", 3, 15, 19,
-    "admin with bypassrls privilege replicates insert into rls enabled $tbl");
-
-  revoke_bypassrls("regress_admin");
-  publish_update("alice.$tbl", 15 => 21);
-  expect_failure("alice.$tbl", 3, 15, 19,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "nobypassrls admin fails to replicate update into rls enabled $tbl");
-
-  grant_bypassrls("regress_admin");
-  expect_replication("alice.$tbl", 3, 17, 21,
-    "admin with restored bypassrls privilege replicates update into rls enabled $tbl");
-
-  revoke_bypassrls("regress_admin");
-  publish_delete("alice.$tbl", 17);
-  expect_failure("alice.$tbl", 3, 17, 21,
-    qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "$tbl\w*"/msi,
-    "nobypassrls admin fails to replicate delete into rls enabled $tbl");
-  grant_bypassrls("regress_admin");
-  expect_replication("alice.$tbl", 2, 19, 21,
-    "admin with restored bypassrls privilege replicates delete into rls enabled $tbl");
-}
+publish_insert("alice.hashpart", 101);
+publish_insert("alice.hashpart", 102);
+publish_insert("alice.hashpart", 103);
+publish_update("alice.hashpart", 102 => 120);
+publish_delete("alice.hashpart", 101);
+expect_replication("alice.hashpart", 2, 103, 120,
+				   "nosuperuser admin with all table privileges can replicate into hashpart");
+
+
+# Enable RLS on the target table and check that "regress_admin" can
+# only replicate into it when superuser or bypassrls.
+#
+$node_subscriber->safe_psql('postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+ALTER TABLE alice.unpartitioned ENABLE ROW LEVEL SECURITY;
+));
+
+revoke_superuser("regress_admin");
+publish_insert("alice.unpartitioned", 15);
+expect_failure("alice.unpartitioned", 2, 11, 13,
+			   qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi,
+			   "non-superuser admin fails to replicate insert into rls enabled table");
+grant_superuser("regress_admin");
+expect_replication("alice.unpartitioned", 3, 11, 15,
+				   "admin with restored superuser privilege replicates insert into rls enabled unpartitioned");
+
+revoke_superuser("regress_admin");
+publish_update("alice.unpartitioned", 11 => 17);
+expect_failure("alice.unpartitioned", 3, 11, 15,
+			   qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi,
+			   "non-superuser admin fails to replicate update into rls enabled unpartitioned");
+
+grant_bypassrls("regress_admin");
+expect_replication("alice.unpartitioned", 3, 13, 17,
+				   "admin with bypassrls replicates update into rls enabled unpartitioned");
+
+revoke_bypassrls("regress_admin");
+publish_delete("alice.unpartitioned", 13);
+expect_failure("alice.unpartitioned", 3, 13, 17,
+			   qr/ERROR:  "regress_admin" cannot replicate into relation with row-level security enabled: "unpartitioned\w*"/msi,
+			   "non-superuser admin without bypassrls fails to replicate delete into rls enabled unpartitioned");
+grant_bypassrls("regress_admin");
+expect_replication("alice.unpartitioned", 2, 15, 17,
+				   "admin with bypassrls replicates delete into rls enabled unpartitioned");
+grant_superuser("regress_admin");
 
 # Alter the subscription owner to "regress_alice".  She has neither superuser
 # nor bypassrls, but as the table owner should be able to replicate.
@@ -346,18 +271,10 @@ sub grant_bypassrls
 ALTER ROLE regress_alice NOSUPERUSER;
 ALTER SUBSCRIPTION admin_sub ENABLE;
 ));
-for my $tbl (@tbl)
-{
-  publish_insert("alice.$tbl", 23);
-  expect_replication(
-    "alice.$tbl", 3, 19, 23,
-    "nosuperuser nobypassrls table owner can replicate insert into $tbl despite rls");
-  publish_update("alice.$tbl", 19 => 25);
-  expect_replication(
-    "alice.$tbl", 3, 21, 25,
-    "nosuperuser nobypassrls table owner can replicate update into $tbl despite rls");
-  publish_delete("alice.$tbl", 21);
-  expect_replication(
-    "alice.$tbl", 2, 23, 25,
-    "nosuperuser nobypassrls table owner can replicate delete into $tbl despite rls");
-}
+
+publish_insert("alice.unpartitioned", 23);
+publish_update("alice.unpartitioned", 15 => 25);
+publish_delete("alice.unpartitioned", 17);
+expect_replication(
+    "alice.unpartitioned", 2, 23, 25,
+    "nosuperuser nobypassrls table owner can replicate delete into unpartitioned despite rls");