Re: On login trigger: take three

On 15.12.2020 20:16, Pavel Stehule wrote:
>
>> Please notice that we still need GUC to disable on-login
>> triggers: to make it possible for superuser who did mistake
>> and defined incorrect on-login trigger to
>> login to the system.
>> Do we need GUC to disable all other event triggers? May be I
>> am wrong, but I do not see much need in such GUC: error in
>> any of such event triggers is non fatal
>> and can be easily reverted.
>> So the only question is whether
>> "disable_client_connection_trigger" should be true by default
>> or not...
>>
>> I agree with you that @2 is a little bit chaotic and @1 looks
>> like a workaround.
>> But from my point of view @3 is not the best solution but
>> overkill: maintaining yet another shared hash just to save
>> few milliseconds on login seems to be too high price.
>> Actually there are many things which are loaded by new
>> backend from the database on start: for example - catalog.
>> This is why launch of new backend is an expensive operation.
>> Certainly if we execute "select 1", then system catalog is
>> not needed...
>> But does anybody start new backend just to execute "select 1"
>> and exit?
>>
>>
>>
>> I understand so the implementation of a new shared cache can be a
>> lot of work. The best way is enhancing pg_database about one
>> column with information about the login triggers
>> (dathaslogontriggers). In init time these data are in syscache,
>> and can be easily checked. Some like pg_attribute have an
>> atthasdef column.  Same fields has pg_class - relhasrules,
>> relhastriggers, ... Then the overhead of this design should be
>> really zero.
>>
>> What do you think about it?
>>
> I like this approach more than implementation of shared hash.
> But still I have some concerns:
>
> 1. pg_database table format has to be changed. Certainly it is not
> something  completely unacceptable. But IMHO we should try to avoid
> modification of such commonly used catalog tables as much as possible.
>
>
> yes, I know. Unfortunately I  don't see any other and correct
> solution. There should be more wide discussion before this work about
> this topic. On second hand, this change should not break anything (if
> this new field will be placed on as the last field). The logon trigger
> really looks like a database trigger - so I think so this semantic is
> correct. I have no idea if it is acceptable for committers :-/. I hope
> so.
>
> The fact that the existence of a logon trigger can be visible from a
> shared database object can be an interesting feature too.
>
>
> 2. It is not so easy to maintain this flag. There can be multiple
> on-login triggers defined. If such trigger is dropped, we can not
> just clear this flag.
> We should check if other triggers exist. Now assume that there are
> two triggers and two concurrent transactions dropping each one.
> According to their snapshots them do not see changes made by other
> transaction. So them remove both triggers but didn't clear the flag.
> Certainly we can use counter instead of flag. But I am not sure
> that their will be not other problems with maintaining counter.
>
>
> I don't think it is necessary.  My opinion is not too strong, but if
> pg_class doesn't need to hold a number of triggers, then I think so
> pg_database doesn't need to hold this number too.
>
Attached please find new versoin of the patch based on
on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch
So there is still only  "disable_client_connection_trigger" GUC? because
we need possibility to disable client connect triggers and there is no
such need for other event types.

As you suggested  have added "dathaslogontriggers" flag which is set
when client connection trigger is created.
This flag is never cleaned (to avoid visibility issues mentioned in my
previous mail).

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company