On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > 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(). > > Nothing in particular but it seems a bit odd to perform operations on > catalog tables with some other user table owners when that was not the > actual intent of this option. > > > 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. > > > > I think that would be better.
Agreed. I've attached the updated patch. Please review it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v3-0001-Fix-tablesync-worker-missed-using-run_as_owner-op.patch
Description: Binary data
