On 15.12.2020 18:25, Pavel Stehule wrote:
út 15. 12. 2020 v 15:06 odesílatel Konstantin Knizhnik
<k.knizh...@postgrespro.ru <mailto:k.knizh...@postgrespro.ru>> napsal:
On 15.12.2020 16:18, Pavel Stehule wrote:
út 15. 12. 2020 v 14:12 odesílatel Konstantin Knizhnik
<k.knizh...@postgrespro.ru <mailto:k.knizh...@postgrespro.ru>>
napsal:
On 11.12.2020 19:27, Pavel Stehule wrote:
pá 11. 12. 2020 v 17:05 odesílatel Konstantin Knizhnik
<k.knizh...@postgrespro.ru
<mailto:k.knizh...@postgrespro.ru>> napsal:
On 11.12.2020 18:40, Pavel Stehule wrote:
is not correct. It makes it not possible to
superuser to disable triggers for all users.
pg_database_ownercheck returns true for superuser always.
Sorry, but I consider different case: when normal user
is connected to the database.
In this case pg_database_ownercheck returns false and
trigger is not disabled, isn't it?
My idea was to reduce necessary rights to database owners.
But you have a true, so only superuser can create event
trigger, so this feature cannot be used in DBaaS
environments, and then my original idea was wrong.
Also GUCs are not associated with any database. So
I do not understand why this check of database
ownership is relevant at all?
What kind of protection violation we want to prevent?
It seems to be obvious that normal user should not
be able to prevent trigger execution because this
triggers may be used to enforce some security policies.
If trigger was created by user itself, then it can
drop or disable it using ALTER statement. GUC is
not needed for it.
when you cannot connect to the database, then you
cannot do ALTER. In DBaaS environments lot of users has
not superuser rights.
But only superusers can set login triggers, right?
So only superuser can make a mistake in this trigger.
But he have enough rights to recover this error. Normal
users are not able to define on connection triggers and
should not have rights to disable them.
yes, it is true
Pavel
--
Konstantin Knizhnik
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company
So what's next?
I see three options:
1. Do not introduce GUC for disabling all event triggers
(especially taken in account that them are disabled by default).
Return back to the patch
on_connect_event_trigger_WITH_SUGGESTED_UPDATES.patch with
"disable_client_connection_trigger"
and make it true by default (to eliminate any overhead for
users which are not using on logintriggers).
2. Have two GUCS: "disable_client_connection_trigger" and
"disable_event_triggers".
3. Implement some mechanism for caching presence of event
triggers in shared memory.
@3 is the best design (the things should work well by default),
@2 is a little bit chaotic and @1 looks like a workaround.
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.
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.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
