Re: On login trigger: take three - Mailing list pgsql-hackers

Hi, Robert!

Thank you for your feedback.

On Tue, Oct 10, 2023 at 5:51 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 10:11 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >  * Hold lock during setting of pg_database.dathasloginevt flag (v32
> > version actually didn't prevent race condition).
>
> So ... how does getting this flag set actually work? And how does
> clearing it work?

I hope I explained that in [1].

> In the case of row-level security, you have to explicitly enable the
> flag on the table level using DDL provided for that purpose. In the
> case of relhas{rules,triggers,subclass} the flag is set automatically
> as a side-effect of some other operation. I tend to consider that the
> latter design is somewhat messy. It's potentially even messier here,
> because at least when you add a rule or a trigger to a table you're
> expecting to take a lock on the table anyway. I don't think you
> necessarily expect creating a login trigger to take a lock on the
> database. That's a bit odd and could have visible side effects. And if
> you don't, then what happens is that if you create two login triggers
> in overlapping transactions, then (1) if there were no login triggers
> previously, one of the transactions fails with an internal-looking
> error message about a concurrent tuple update and (2) if there were
> login triggers previously, then it works fine.

Yep, in v43 it worked that way.  One transaction has to wait for
another finishing update of pg_database tuple, then fails.  This is
obviously ridiculous.  Since overlapping setters of flag will have to
wait anyway, I changed lock mode in v44 for them to
AccessExclusiveLock.  Now, waiting transaction then sees the updated
tuple and doesn't fail.

> That's also a bit weird
> and surprising. Now the counter-argument could be that adding new DDL
> to enable login triggers for a database is too much cognitive burden
> and it's better to have the kind of weird and surprising behavior that
> I just discussed. I don't know that I would buy that argument, but it
> could be made ... and my real point here is that I don't even see
> these trade-offs being discussed. Apologies if they were discussed
> earlier and I just missed that; I confess to not having read every
> email message on this topic, and some of the ones I did read I read a
> long time ago.

I have read the thread quite carefully.  I don't think manual setting
of the flag was discussed.  I do think it would be extra burden for
users, and I would prefer automatic flag as long as it works
transparently and reliably.

> > This version should be good and has no overhead.  Any thoughts?
> > Daniel, could you please re-run the performance tests?
>
> Is "no overhead" an overly bold claim here?

Yes, sure.  I meant no extra lookup.  Hopefully that would mean no
measurable overhead when is no enabled login triggers.

Links
1. https://www.postgresql.org/message-id/CAPpHfdtvvozi%3Dttp8kvJQwuLrP5Q0D_5c4Pw1U67MRXcROB9yA%40mail.gmail.com

------
Regards,
Alexander Korotkov