On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@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())?
>
> 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?
+# Because the initial data sync is working as the table owner, all
+# dat should be copied.
Typo. /dat/data
--
With Regards,
Amit Kapila.
