On Tue, 2021-12-14 at 07:41 -0800, Mark Dilger wrote:
> I went a different direction with this. The need is to prevent non-
> superuser subscription owners from *circumventing* RLS. For this
> patch set, I'm just checking whether RLS should be enforced against
> the subscription owner, and if so, raising an ERROR, same as for a
> privilege violation. This should be sufficient in practice, as the
> table owner, roles with bypassrls privilege, and superusers should
> still be able to replicate into an RLS enabled table.
I agree with this. Let's consider subscription targets involving RLS a
separate feature that is just blocked for now (except for bypassrls
roles and superusers).
> As far as completeness and a clean implementation (but not
> performance) are concerned, using ExecInsert is quite appealing. I
> think that would require reworking the logical replication protocol
> to send more than one row at a time, so that the overhead of such a
> choice is not paid *per row*. That seems quite out of scope for this
> release cycle, though.
Are you sure overhead is a problem? INSERT INTO ... SELECT goes through
that path. And the existing logical replication insert path does some
extra stuff, like opening the table for each row, so it's not clear to
me that logical replication is much faster.
Also, the current patch does per-row ACL checks. Did you consider
making the checks a part of logicalrep_rel_open()? That already has a
path to consider relcache invalidation, so it would also be a good
place to check if the table has RLS enabled.
Regards,
Jeff Davis