Re: limiting connections per user/database

Hello,

patch I attached allows to set connectin limits per user or per database
(it's on TODO list).
It's proposal because I am not sure if this implementation can be
accepted (I never made new feature patch for postgres so I really dunno)
and I would like to know what you guys think about it and what I should
change.

Something about patch:
I added two new guc variables name max_db_connections and
max_user_connections which can be set by superuser which means it can be
in main config file or in user/database config.
I was thinking about 3 different aproaches - the other two was using
max_connections with set/get hooks or change in catalog tables but new
fuc variables seemed best solution to me.

Conenction limits are checked in InitPostgres function after user and
database configs are loaded.
Patch works only when stats are on because it takes number of
connections per user and database from there - I had to patch pgstat to
store user connection stats.

Also this patch relies on bugfix I sent on Thursday but I wasn't
subcribed and it still waits for moderation so I attached it to this
mail too (pgstat.c.diff) because without it database stats are broken in
current CVS.

I modified only .c sources, no documentation, I will make documentation
changes when (and if) this will be finished and accepted.

Diffs should be against latest CVS.

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelínek said:
>
> Something about patch:
> I added two new guc variables name max_db_connections and
> max_user_connections which can be set by superuser which means it can
> be in main config file or in user/database config.

Is this what is intended by the TODO item? I thought that it was intended to
allow max connections to be specified on a per-db or per-user basis, not
just for global limits on per-user or per-db connections.

cheers

andrew

=?ISO-8859-2?Q?Petr_Jel=EDnek?= <pjmodos(at)parba(dot)cz> writes:
> patch I attached allows to set connectin limits per user or per database
> (it's on TODO list).

I don't think this is going to work. The first problem with it is that
it changes pgstats from an optional into a required part of the system.
The second is that since the pgstats output lags well behind the actual
state of the system, it'll be at best a very approximate limit on the
number of connections. Perhaps for some people that's good enough, but
I kinda doubt it's what the people asking for the feature have in mind.

regards, tom lane

Ok didn't know that stats are well behind actual state - I used pgstat
because it already stores number of conenctions per database so I
thought doing same thing again is needless.
So you think I should store those data somewhere myself. What you think
is best place, some extra hash or in system catalog or ... ?

BTW you should still apply that pgstat.c.diff because it fixes bug in
current CVS

--
Regards
Petr Jelinek (PJMODOS)
www.parba.cz

Andrew Dunstan wrote:

>Is this what is intended by the TODO item? I thought that it was intended to
>allow max connections to be specified on a per-db or per-user basis, not
>just for global limits on per-user or per-db connections.
>
>
They are - ALTER DATABASE dbname SET max_db_conenctions = '10';
Like I said they're checked after user specific and database specific
config is loaded.
But after what Tom said I think I will have to rewrite it all so it
doesn't really matter.

--
Regards
Petr Jelinek (PJMODOS)

=?windows-1250?Q?Petr_Jel=EDnek?= <pjmodos(at)parba(dot)cz> writes:
> BTW you should still apply that pgstat.c.diff because it fixes bug in
> current CVS

What bug exactly?

regards, tom lane

Tom Lane wrote:

> What bug exactly?
>
Database stats aren't initialized so everything in pg_stat_database is
always zero.

--
Regards
Petr Jelinek (PJMODOS)

=?windows-1250?Q?Petr_Jel=EDnek?= <pjmodos(at)parba(dot)cz> writes:
> Tom Lane wrote:
>> What bug exactly?
>>
> Database stats aren't initialized so everything in pg_stat_database is
> always zero.

[ shrug... ] Don't see that here; sure it isn't something broken in
your local modified version?

regards, tom lane

Tom Lane napsal(a):

>[ shrug... ] Don't see that here; sure it isn't something broken in
>your local modified version?
>
>
Well I tryed it with unmodified cvs copy and I have numbackends is 0
when 40 clients are connected (using default config)

I am bit confused now because I am no really sure if it's intended to be
this way or not - 8.0 behaviour was to report numbackends when stats
were on, now it reports numbackends when stats_row_level is true.
I should prolly talk with neilc to see what he intended to do when he
commited code which changed this (but I believe it's side effect not
intended change)

--
Regards
Petr Jelinek (PJMODOS)

One more thing, do you have more concerns about that max connection out
of the fact it uses pgstat ?
For example are guc variables ok for this or should it be new variable
in pg_shadow & pg_database etc - I would like to rewrite it just once or
twice not ten times.

--
Regards
Petr Jelinek (PJMODOS)

Hi Petr,

> One more thing, do you have more concerns about that max connection
> out
> of the fact it uses pgstat ?
> For example are guc variables ok for this or should it be new
> variable
> in pg_shadow & pg_database etc - I would like to rewrite it just once
> or
> twice not ten times.
>
IIRC, people want the last one. We have more control if we can set it
per user or per database. Talking about pgstat, I think you can rely on
it 'cause it can be disabled. Could you describe the design you
intended to implement?

Euler Taveira de Oliveira
euler[at]yahoo_com_br

__________________________________________________
Converse com seus amigos em tempo real com o Yahoo! Messenger
http://br.download.yahoo.com/messenger/

Euler Taveira de Oliveira wrote:

>IIRC, people want the last one. We have more control if we can set it
>per user or per database.
>
Like I said, guc variables _can_ be set per user or per database if you
use right context.

>Talking about pgstat, I think you can rely on
>it 'cause it can be disabled. Could you describe the design you
>intended to implement?
>
>
I "can rely on it 'cause it can be disabled" - did you mean that I can't
rely on it ?
Well I am still not sure how I will implement it when I can't use
pgstat. I think that I will have to use similar aproach which makes me sad.

--
Regards
Petr Jelinek (PJMODOS)

On Sun, Jun 26, 2005 at 07:46:32PM +0200, Petr Jelínek wrote:
> Euler Taveira de Oliveira wrote:
>
> >IIRC, people want the last one. We have more control if we can set it
> >per user or per database.
>
> Like I said, guc variables _can_ be set per user or per database if you
> use right context.

I don't think this approach is very user-friendly. I'd vote for the
catalog approach, I think.

> >Talking about pgstat, I think you can rely on
> >it 'cause it can be disabled. Could you describe the design you
> >intended to implement?
>
> I "can rely on it 'cause it can be disabled" - did you mean that I can't
> rely on it ?
> Well I am still not sure how I will implement it when I can't use
> pgstat. I think that I will have to use similar aproach which makes me sad.

Maybe you could make some checks against the shared array of PGPROCs
(procarray.c), for the per-database limit at least. Not sure about
per-user limit.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"[PostgreSQL] is a great group; in my opinion it is THE best open source
development communities in existence anywhere." (Lamar Owen)

Alvaro Herrera wrote:

>I don't think this approach is very user-friendly. I'd vote for the
>catalog approach, I think.
>
>
Ok I am fine with both but catalog changes would mean more hacking of
ALTER DATABASE and ALTER USER.

>Maybe you could make some checks against the shared array of PGPROCs
>(procarray.c), for the per-database limit at least. Not sure about
>per-user limit.
>
>
Thats good idea (I could maybe add userid to PGPROC struct too) but I
think there could be problem with two phase commits because they add new
entry to that array of PGPROCs too and I don't kow if we want to include
them to that limit.

--
Regards
Petr Jelinek (PJMODOS)

On Sun, 26 Jun 2005, Petr Jelínek wrote:

> Alvaro Herrera wrote:
>
>> Maybe you could make some checks against the shared array of PGPROCs
>> (procarray.c), for the per-database limit at least. Not sure about
>> per-user limit.
>>
> Thats good idea (I could maybe add userid to PGPROC struct too) but I think
> there could be problem with two phase commits because they add new entry to
> that array of PGPROCs too and I don't kow if we want to include them to that
> limit.

You can ignore PGPROCs that belong to prepared transactions. They have 0
in the pid field, see TransactionIdIsActive and
CountActiveBackends functions in procarray.c for an example.

- Heikki

On Sun, Jun 26, 2005 at 08:52:55PM +0200, Petr Jelínek wrote:
> Alvaro Herrera wrote:

> >Maybe you could make some checks against the shared array of PGPROCs
> >(procarray.c), for the per-database limit at least. Not sure about
> >per-user limit.
>
> Thats good idea (I could maybe add userid to PGPROC struct too) but I
> think there could be problem with two phase commits because they add new
> entry to that array of PGPROCs too and I don't kow if we want to include
> them to that limit.

Prepared transactions can be filtered out by checking the pid struct
member. I'm not sure if anybody would object to adding the
authenticated user Id to ProcArray, but I don't see why not.

--
Alvaro Herrera (<alvherre[a]surnet.cl>)
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
That's because in Europe they call me by name, and in the US by value!"

Alvaro Herrera wrote:

>Prepared transactions can be filtered out by checking the pid struct
>member. I'm not sure if anybody would object to adding the
>authenticated user Id to ProcArray, but I don't see why not.
>
>
Very well, it seems to work this way (although the code for storing
userid in PGPROC isn't as clean as I hoped).

The problem now is that I am storing those limits in system catalog but
there is no ALTER DATABASE implementation which I could use to change
this - there are only RENAME, OWNER and SET implementations and those
are not usable for "normal" properties (SET is for guc variables only
which was the actual reason for me to use guc variables first time).
That said I think that I will have to implement some ALTER DATABASE
command for those purposes and I am not sure if I can handle it because
I am not familiar with bison (I have even problems to add new ALTER USER
property to existing implementation).

--
Regards
Petr Jelinek (PJMODOS)

Petr Jelínek wrote:
> I am bit confused now because I am no really sure if it's intended to be
> this way or not - 8.0 behaviour was to report numbackends when stats
> were on, now it reports numbackends when stats_row_level is true.

Yeah, this is a bug. Attached is a fix. I'll apply this to HEAD later
today barring any objections.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> Yeah, this is a bug. Attached is a fix. I'll apply this to HEAD later
> today barring any objections.

I looked at this but did not actually see the code path that requires
forcing creation of the per-DB entry right at this spot. The HASH_FIND
calls for this hashtable seem to all happen on the backend side not the
collector side. Can you explain why we need this?

regards, tom lane

Tom Lane wrote:
> I looked at this but did not actually see the code path that requires
> forcing creation of the per-DB entry right at this spot. The HASH_FIND
> calls for this hashtable seem to all happen on the backend side not the
> collector side. Can you explain why we need this?

Yeah, I missed this when making the original change (this code is rather
opaque :-\). The problem is that if we don't initialize the dbentry for
the database we connect to, it won't get written out to the statsfile in
pgstat_write_statsfile(). So the database won't be counted as having any
backends connected to it in pgstat_read_statsfile() (see line 2558 of
pgstat.c in HEAD).

BTW, the comment at line 2210 of pgstat.c is misleading: the n_backends
in the entries of the dbentry hash table are explicitly ignored when
reading in the stats file -- the value is instead derived from the
number of beentries that are seen.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> Tom Lane wrote:
>> I looked at this but did not actually see the code path that requires
>> forcing creation of the per-DB entry right at this spot. The HASH_FIND
>> calls for this hashtable seem to all happen on the backend side not the
>> collector side. Can you explain why we need this?

> Yeah, I missed this when making the original change (this code is rather
> opaque :-\).

No kidding. Somebody ought to separate the collector-side code from the
backend-side code sometime.

> BTW, the comment at line 2210 of pgstat.c is misleading: the n_backends
> in the entries of the dbentry hash table are explicitly ignored when
> reading in the stats file -- the value is instead derived from the
> number of beentries that are seen.

Right. So do we care whether the collector has the right number?
Or should we push the responsibility for tracking that count over
to the collector (+1 for that personally)?

regards, tom lane

Tom Lane wrote:
> Right. So do we care whether the collector has the right number?

Not at present; n_backends is not read/written by the stats collector
itself, it is just in the hash table for the convenience of backends who
read in the stats file.

> Or should we push the responsibility for tracking that count over
> to the collector (+1 for that personally)?

Makes sense to me -- I'll take a look at implementing this. For now I'll
just commit the bug fix.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> Makes sense to me -- I'll take a look at implementing this. For now I'll
> just commit the bug fix.

I'm still missing what the exact "bug fix" is here. So far we have
established that (a) the n_backends count is not tracked by the
collector; (b) the collector does not need it; (c) the count is
recomputed by each backend that might need it; and (d) we could probably
save some cycles by letting the collector count active backends instead
of making the backends do it.

(d) is a performance bug, but if there is a functionality bug I'm not
seeing it.

regards, tom lane

Tom Lane wrote:
> I'm still missing what the exact "bug fix" is here.

The bug is:

- a backend starts up and sends the collector a BESTART message. For the
sake of clarity, suppose that the backend is the first and only backend
connected to its database.

- the stats collector receives the BESTART message and records the
existence of the backend via pgstat_add_backend(). In current sources,
it does not initialize the entry for the backend's database in pgStatDBHash.

- the stats collector then decides to write out the stats file. Since
there is no pgStatDBHash entry for the backend's database, we don't
write out anything for it.

- when we read in the stats collector's stats file in a normal backend,
there will be no pgStatDBHash entry for the backend's database.
Therefore we'll read the beentry for the backend, and when we go to
increment the n_backends for the corresponding dbentry, there will be no
dbentry found (HASH_FIND at pgstat.c:2554), so n_backends won't be updated.

- therefore we won't count the n_backends for the database correctly.

This can be seen in current sources with a fresh initdb and default
postgresql.conf settings: connect to a database, and do select * from
pg_stat_database.

-Neil

Tom Lane wrote:

>(d) is a performance bug, but if there is a functionality bug I'm not
>seeing it.
>
>
You must use default config (or atleast turn off everything in stats
thats off by default) to see this bug.

And if you guys want to be more confused then when i'll make that
conenction limit patch (and if it gets accepted) then there will be
function to get exact number of backends for database (atlest thats my
current aproach) so whole n_backend thingy will not be needed anymore :)

--
Regards
Petr Jelinek (PJMODOS)

So now I have it all working including brand new alter database (this
would have to be doublechecked by somebody from core team) and using
proc array to get number of backends per database and per user.

My only concer now is that userid in PGPROC, does anybody have anything
against it ?

And one more thing about userid, at the time MyProc is inserted to proc
array we don't know userid, so I am now updating MyProc as soon as I
know it (check for connection limit is done afterwards).
There is another solution I know of - I could use flatfiles to get
userid before MyProc is stored in proc array (which is how we get
MyDatabaseId anyway).
What of those solutions is better in your opinion ?

Oh and atm those limits are not enforced for superusers, any objections ?

--
Regards
Petr Jelinek (PJMODOS)
www.parba.cz

Neil Conway <neilc(at)samurai(dot)com> writes:
> - when we read in the stats collector's stats file in a normal backend,
> there will be no pgStatDBHash entry for the backend's database.
> Therefore we'll read the beentry for the backend, and when we go to
> increment the n_backends for the corresponding dbentry, there will be no
> dbentry found (HASH_FIND at pgstat.c:2554), so n_backends won't be updated.

> - therefore we won't count the n_backends for the database correctly.

However, this could equally be fixed by replacing the hash_search call
at l. 2554 by pgstat_get_db_entry(). If your definition of "correct"
is that "a new backend can see at least one backend in its own database"
then this would be a more appropriate fix anyway, since there's a lag
between sending BESTART and seeing any result in the collector's output.

This isn't an argument against moving the responsibility for counting
n_backends into the collector, but as long as it's done in
pgstat_read_statsfile the proposed fix is pretty bogus.

regards, tom lane

On Sun, 2005-06-26 at 20:52 +0200, Petr Jelínek wrote:
> Alvaro Herrera wrote:
>
> >I don't think this approach is very user-friendly. I'd vote for the
> >catalog approach, I think.
> >
> >
> Ok I am fine with both but catalog changes would mean more hacking of
> ALTER DATABASE and ALTER USER.

IMHO Oracle has better solution for per-user setting:

CREATE PROFILE <name>
SESSIONS_PER_USER <int>
CONNECT_TIME <int>
IDLE_TIME <int>;

ALTER USER <name> PROFILE <name>;

karel

--
Karel Zak <zakkr(at)zf(dot)jcu(dot)cz>