On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > Thank you for updating the patch! Here are review comments: > > > > + /* > > + * Make sure that the copy command runs as the table owner, unless > > + * the user has opted out of that behaviour. > > + */ > > + run_as_owner = MySubscription->runasowner; > > + if (!run_as_owner) > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > + > > /* Now do the initial data copy */ > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > I think we should switch users before the acl check in > > LogicalRepSyncTableStart(). > > > > Agreed, we should check acl with the user that is going to perform > operations on the target table. BTW, is it okay to perform an > operation on the system table with the changed user as that would be > possible with your suggestion (see replorigin_create())?
Do you see any problem in particular? As per the documentation, pg_replication_origin_create() is only allowed to the superuser by default, but in CreateSubscription() a non-superuser (who has pg_create_subscription privilege) can call replorigin_create(). OTOH, we don't necessarily need to switch to the table owner user for checking ACL and RLS. We can just pass either table owner OID or subscription owner OID to pg_class_aclcheck() and check_enable_rls() without actually switching the user. > > > > > While this approach works, I'm not sure we really need a trigger for > > this test. I've attached a patch for discussion that doesn't use > > triggers for the regression tests. We create a new subscription owned > > by a user who doesn't have the permission to the target table. The > > test passes only if run_as_owner = true works. > > > > Why in the test do you need to give additional permissions to > regress_admin2 when the actual operation has to be performed by the > table owner? Good point. We need to give the ability to SET ROLE to regress_admin2 but other permissions are unnecessary. > > +# Because the initial data sync is working as the table owner, all > +# dat should be copied. > > Typo. /dat/data Will fix. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
