From: Tom Lane <tgl@sss.pgh.pa.us>
To: Laurenz Albe <laurenz.albe@cybertec.at>
cc: Pavel Stehule <pavel.stehule@gmail.com>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Wrong security context for deferred triggers?
In-reply-to: <51386e72c47d922ef68eaa67451a52dc3f569843.camel@cybertec.at>
References: <77ee784cf248e842f74588418f55c2931e47bd78.camel@cybertec.at>
 <dede297a-3d24-7029-dfd9-06aeef8b9766@enterprisedb.com>
 <6f27f51bc98354d7254bbde1c171fa13aca03f28.camel@cybertec.at>
 <77b89e609f21380785865542609fbc14010021c8.camel@cybertec.at>
 <CAFj8pRDcE3+9EM+PSSEW1bHA-EWMAYgVog41q0c47N=Pnq_qiQ@mail.gmail.com>
 <3f0a08f3e3d445137ae287125bc8c1a007045db8.camel@cybertec.at>
 <CAFj8pRCCdL_vB-42161CWxemBVqF75WOq53cMnmRCrzjvKpLOQ@mail.gmail.com>
 <66021ca9d81c4e1a22b6c417659b05424e130f27.camel@cybertec.at>
 <CAFj8pRB1UVUuEh_T7j7XFo8yYDuEYxoUgMhPXbny6dK1eCvgRg@mail.gmail.com>
 <6bca669972ef0d4a665fa83c5b751ab116324f8f.camel@cybertec.at>
 <CAFj8pRDcy2OaPS7OtceFRhTRq6VKWbdnuawgaX3EXMQVwTR4VA@mail.gmail.com>
 <2653121.1730737337@sss.pgh.pa.us>
 <51386e72c47d922ef68eaa67451a52dc3f569843.camel@cybertec.at>
Comments: In-reply-to Laurenz Albe <laurenz.albe@cybertec.at>
	message dated "Mon, 04 Nov 2024 18:25:51 +0100"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3944968.1737570355.1@sss.pgh.pa.us>
Date: Wed, 22 Jan 2025 13:25:55 -0500
Message-ID: <3944969.1737570355@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3944969.1737570355%40sss.pgh.pa.us>
Precedence: bulk

Laurenz Albe <laurenz.albe@cybertec.at> writes:
> [ v4-0001-Make-AFTER-triggers-run-with-the-correct-user.patch ]

I started to look at this and got distracted by wondering why
afterTriggerAddEvent's scanning loop wasn't checking ats_modifiedcols.
That led to ea68ea632, which means this now needs a minor rebase.

I have a couple other thoughts:

* It's kind of sad that we have to add yet another field to
AfterTriggerSharedData, especially since this one will cost 8 bytes
thanks to alignment considerations.  I'm not sure if there's another
way, but it seems like ats_relid, ats_rolid, and ats_modifiedcols
are all going to be extremely redundant in typical scenarios.
Maybe it's worth rethinking that data structure a bit?  Or maybe
I'm worried over nothing; perhaps normal situations have only a few
AfterTriggerSharedDatas anyway.  It might be worth trying to gather
some statistics about that.

* I don't buy the bit about "We have to check if the role still
exists"; I think that's just a waste of cycles.  There is no check
that prevents dropping the role a session is currently running as,
so why do we need one here?  Moreover, the role could still get
dropped immediately after you look.  Or for that matter, save_rolid
could be invalid by the time you restore it.  None of this bothers me,
because if a session is running as a dropped role it will still
function pretty normally.  It will find that it has no privileges
beyond those granted to PUBLIC, and if it tries to inspect
current_user or related functions those will fail, but there's
no compromise to system integrity.  So I'd just remove that test.

			regards, tom lane