Re: Wrong security context for deferred triggers?

On Wed, 2025-01-22 at 13:25 -0500, Tom Lane wrote:
> 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.

Thanks for your efforts!

> 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.

Thanks for investigating that as well.
That matches my intuition that you typically don't run hundreds of
different AFTER triggers in a single transaction.
What about partitioned tables with hundreds of partitions?  Could
that be a problem?  But then 100 times 8 bytes is still not a lot
these days.

> * 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.

That makes sense to me.  I was worried that it may lead to cryptic,
hard-to-debug errors later on.

Version 5 of the patch is attached.

Yours,
Laurenz Albe