Обсуждение: SQL access to database attributes

We try to tell our clients not to update the catalogs directly, but
there are at least two instances where it's not possible to do otherwise
(pg_database.datistemplate and .datallowconn).  This patch aims to
remedy that.

For example, it is now possible to say
    ALTER DATABASE d ALLOW CONNECTIONS = false;
and
    ALTER DATABASE d IS TEMPLATE = true;

This syntax matches that of CONNECTION LIMIT but unfortunately required
me to make ALLOW and CONNECTIONS unreserved keywords.  I know we try not
to do that but I didn't see any other way.  The two new options are of
course also available on CREATE DATABASE.

There is a slight change in behavior with this patch in that previously
one had to be superuser or have rolcatupdate appropriately set, and now
the owner of the database is also allowed to change these settings.  I
believe this is for the better.

It was suggested to me that these options should either error out if
there are existing connections or terminate said connections.  I don't
agree with that because there is no harm in connecting to a template
database (how else do you modify it?), and adding a reject rule in
pg_hba.conf doesn't disconnect existing users so why should turning off
ALLOW CONNECTIONS do it?

As for regression tests, I couldn't figure out how to make CREATE/ALTER
DATABASE play nice with make installcheck and so I haven't provided any.

Other than that, I think this patch is complete and so I'm adding it the
next commitfest.

--
Vik

On Fri, May 23, 2014 at 10:53 PM, Vik Fearing <vik.fearing@dalibo.com> wrote:
>
> It was suggested to me that these options should either error out if
> there are existing connections or terminate said connections.  I don't
> agree with that because there is no harm in connecting to a template
> database (how else do you modify it?), and adding a reject rule in
> pg_hba.conf doesn't disconnect existing users so why should turning off
> ALLOW CONNECTIONS do it?
>

Which lead us to the question: you need to connect to the database to
modify it, don't you? then, how do you change ALLOW CONNECTIONS to
true?

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566         Cell: +593 987171157

On 05/24/2014 12:03 AM, Jaime Casanova wrote:
> On Fri, May 23, 2014 at 10:53 PM, Vik Fearing <vik.fearing@dalibo.com> wrote:
>> It was suggested to me that these options should either error out if
>> there are existing connections or terminate said connections.  I don't
>> agree with that because there is no harm in connecting to a template
>> database (how else do you modify it?), and adding a reject rule in
>> pg_hba.conf doesn't disconnect existing users so why should turning off
>> ALLOW CONNECTIONS do it?
>>
> Which lead us to the question: you need to connect to the database to
> modify it, don't you? then, how do you change ALLOW CONNECTIONS to
> true?

You can ALTER DATABASE from anywhere.

-- 
Vik

On Fri, May 23, 2014 at 11:06 PM, Vik Fearing <vik.fearing@dalibo.com> wrote:
> On 05/24/2014 12:03 AM, Jaime Casanova wrote:
>> On Fri, May 23, 2014 at 10:53 PM, Vik Fearing <vik.fearing@dalibo.com> wrote:
>>> It was suggested to me that these options should either error out if
>>> there are existing connections or terminate said connections.  I don't
>>> agree with that because there is no harm in connecting to a template
>>> database (how else do you modify it?), and adding a reject rule in
>>> pg_hba.conf doesn't disconnect existing users so why should turning off
>>> ALLOW CONNECTIONS do it?
>>>
>> Which lead us to the question: you need to connect to the database to
>> modify it, don't you? then, how do you change ALLOW CONNECTIONS to
>> true?
>
> You can ALTER DATABASE from anywhere.
>

ah! doh! right!
don't know why i was convinced you need to connect to the database to
execute ALTER DATABASE

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566         Cell: +593 987171157

Vik Fearing <vik.fearing@dalibo.com> writes:
> On 05/24/2014 12:03 AM, Jaime Casanova wrote:
>> Which lead us to the question: you need to connect to the database to
>> modify it, don't you? then, how do you change ALLOW CONNECTIONS to
>> true?

> You can ALTER DATABASE from anywhere.

Perhaps it'd be wise to have a safety check to disallow turning off
datallowconn for the last connectable database?  Although it couldn't be
bulletproof due to race conditions, so maybe that'd just be nannyism.
(If you do shoot yourself in the foot that way, I think we ignore
datallowconn in standalone mode.)

As with the patch we were discussing yesterday, -1 for inventing new
parser keywords for this.  I wonder if we couldn't refactor the grammar
so it thinks all of CREATE DATABASE's "WITH" options are "identifier =
value" and none of them have to be keywords.
        regards, tom lane

On 5/24/14, 8:14 AM, Tom Lane wrote:
> Vik Fearing<vik.fearing@dalibo.com>  writes:
>> >On 05/24/2014 12:03 AM, Jaime Casanova wrote:
>>> >>Which lead us to the question: you need to connect to the database to
>>> >>modify it, don't you? then, how do you change ALLOW CONNECTIONS to
>>> >>true?
>> >You can ALTER DATABASE from anywhere.
> Perhaps it'd be wise to have a safety check to disallow turning off
> datallowconn for the last connectable database?  Although it couldn't be
> bulletproof due to race conditions, so maybe that'd just be nannyism.
> (If you do shoot yourself in the foot that way, I think we ignore
> datallowconn in standalone mode.)

I think this is nannyism that would be well placed. Most people don't know about standalone, and I don't think we want
tochange that.
 

BTW, I think the race condition could be eliminated if we did something like (forgive the user-space semantics):

SELECT datallowconn FROM pg_database WHERE datallowconn AND datname <> $$database we're disallowing connections on$$
LIMIT1 FOR UPDATE;
 

If you don't get a record back from that you abort; meanwhile no one else can disallow connections on that database
untilyou commit or rollback.
 
-- 
Jim C. Nasby, Data Architect                       jim@nasby.net
512.569.9461 (cell)                         http://jim.nasby.net

Jim Nasby <jim@nasby.net> writes:
> On 5/24/14, 8:14 AM, Tom Lane wrote:
>> Perhaps it'd be wise to have a safety check to disallow turning off
>> datallowconn for the last connectable database?  Although it couldn't be
>> bulletproof due to race conditions, so maybe that'd just be nannyism.

> BTW, I think the race condition could be eliminated if we did something like (forgive the user-space semantics):

> SELECT datallowconn FROM pg_database WHERE datallowconn AND datname <> $$database we're disallowing connections on$$
LIMIT1 FOR UPDATE;
 

> If you don't get a record back from that you abort; meanwhile no one else can disallow connections on that database
untilyou commit or rollback.
 

Meh.  That would take out a rowlock on a database unrelated to the one
we're modifying, which would be (a) surprising and (b) subject to
deadlocks.

I don't really object to doing an unlocked check for another such
database, but I'm not convinced that additional locking to try to
prevent a race is worth its keep.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I don't really object to doing an unlocked check for another such
> database, but I'm not convinced that additional locking to try to
> prevent a race is worth its keep.

+1 on the nannyism, and +1 to ignoring the race.
Thanks,
    Stephen

On 05/26/2014 07:10 AM, Stephen Frost wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I don't really object to doing an unlocked check for another such
>> database, but I'm not convinced that additional locking to try to
>> prevent a race is worth its keep.
> +1 on the nannyism, and +1 to ignoring the race.

Okay, I'll submit a new patch with racy nannyism and some grammar
changes that Tom and I have been discussing in private.

-- 
Vik

On 05/26/2014 08:19 PM, Vik Fearing wrote:
> On 05/26/2014 07:10 AM, Stephen Frost wrote:
>> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>>> I don't really object to doing an unlocked check for another such
>>> database, but I'm not convinced that additional locking to try to
>>> prevent a race is worth its keep.
>> +1 on the nannyism, and +1 to ignoring the race.
> Okay, I'll submit a new patch with racy nannyism and some grammar
> changes that Tom and I have been discussing in private.

Attached are two patches.

The first is a refactoring of the createdb/alterdb grammars mostly by
Tom which makes all of the options non-keywords that don't otherwise
need to be.  Not only does this remove the two unreserved keywords I had
added (ALLOW and CONNECTIONS) but also removes two existing ones
(LC_COLLATE and LC_TYPE), reducing gram.o by about half a percent by
Tom's calculations.  That's much better than increasing it like my
original patch did.

The problem is we foolishly adopted a two-word option name (CONNECTION
LIMIT) which complicates the grammar.  My aping of that for IS TEMPLATE
and ALLOW CONNECTIONS only aggravated the situation.  And so I changed
all the documentation (and pg_dumpall etc) to use CONNECTION_LIMIT
instead.  We might hopefully one day deprecate the with-space version so
the sooner the documentation recommends the without-space version, the
better.  The old syntax is of course still valid.

I also changed the documentation to say connection_limit instead of
connlimit.  Documentation is for humans, something like "connlimit" (and
later as we'll see allowconn) is for programmers.  It also indirectly
reminds us that we should not add another multi-word option like I
initially did.

Now that anything goes grammar-wise, the elog for unknown option is now
ereport.

I am hoping this gets backpatched.

-------

The second patch adds my original functionality, except this time the
syntax is IS_TEMPLATE and ALLOW_CONNECTIONS (both one word, neither
being keywords).  It also changes all the catalog updates we used to do
because we didn't have this patch.

As for the nannyism, the point was to find another database having
datallowconn=true but since we have to be connected to a database to
issue this command, the simplest is just to disallow the current
database (credit Andres on IRC) so that's what I've done as explained
with this in-code comment:

        /*
        * In order to avoid getting locked out and having to go through
standalone
        * mode, we refuse to disallow connections on the database we're
currently
        * connected to.  Lockout can still happen with concurrent
sessions but the
        * likeliness of that is not high enough to worry about.
        */

I also changed the C variable connlimit to dbconnlimit in
AlterDatabase() to be consistent with its analog in createdb().

-------

The third and non-existent patch is about regression tests.  Tom put in
gram.y that we should have some now that the grammar doesn't regulate
it, and I wanted some anyway in my original patch; but I don't know what
they should look like or where they should go so I'm asking for help on
that.

--
Vik