Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: logical replication of truncate command with trigger causes
 Assert
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <1250962.1623192946@sss.pgh.pa.us>
Date: Tue, 8 Jun 2021 16:23:47 -0700
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
 Simon Riggs <simon.riggs@enterprisedb.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <CBED95D4-5072-4A16-9271-72AB73478817@enterprisedb.com>
References: <B4A3AF82-79ED-4F4C-A4E5-CD2622098972@enterprisedb.com>
 <1250962.1623192946@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/CBED95D4-5072-4A16-9271-72AB73478817%40enterprisedb.com>
Precedence: bulk


> On Jun 8, 2021, at 3:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> 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.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company