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
