> On Jun 8, 2021, at 3:55 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> The right way to say that is "commit 84f5c2908da exposed the pre-existing
> unsafe behavior of this code". It's not okay to run arbitrary user code
> without holding a snapshot to protect TOAST dereference operations.
Sure, I didn't expect that you'd broken things so much as we now have an Assert
where, at least for simple commands, things were working back in April. Those
things may not have been working correctly -- I'll have to do some more test
development to see if I can get the pre-84f5c2908da to misbehave -- but this
may appear to be a regression in version 14 if we don't do something.
Calling ExecuteTruncateGuts from within the logical replication worker was
introduced in commit 039eb6e92f2, "Logical replication support for TRUNCATE",
back in April 2018. So whatever we do will likely need to be backpatched.
> I suppose that either apply_dispatch or LogicalRepApplyLoop needs to
> grow some more snapshot management logic, but I've not looked at that
> code much, so I don't have an opinion on just where to add it.
I was looking at those for other reasons prior to hitting this bug. My purpose
was to figure out how to get the code to respect privileges. Perhaps the
solution to these two issues is related. I don't know yet.
As it stands, a subscription can only be created by a superuser, and the
replication happens under that user's current_user and session_user. I naively
thought that adding a built-in role pg_logical_replication which could create
subscriptions would be of some use. I implemented that but, but now if I
create a user named "replication_manager" with membership in
pg_logical_replication but not superuser, it turns out that even though the
apply worker runs as replication_manager, the insert/update/delete commands
work without checking privileges. (They can insert/update/delete tables and
execute functions owned by a database superuser for which "replication_manager"
has no privileges.) So I need to go a bit further to get acl checks called
from this code path.
> There's a reasonable case to be made that running user code outside
> a Portal is a just-plain-bad idea, so we should fix the logical
> apply worker to make it create a suitable Portal. I speculated about
> that in the commit message for 84f5c2908da, but I don't feel like
> taking point on such work.
I'll dig into this a bit more.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
