Re: SQL access to database attributes

Hello

I am looking createdb_alterdb_grammar_refactoring.v1.patch

http://www.postgresql.org/message-id/53868E57.3030908@dalibo.com

Is any reason or is acceptable incompatible change CONNECTION_LIMIT instead
CONNECTION LIMIT? Is decreasing parser size about 1% good enough for
breaking compatibility?

Surely this patch cannot be backported what is proposed there.

Regards

Pavel

Second question related to second patch:

Must be new syntax ALLOW_CONNECTIONS? Should not be (ENABLE | DISABLE)
CONNECTION ? This doesn't need any new keyword.

Regards

Pavel

2014-06-21 22:11 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hello
>
> I am looking createdb_alterdb_grammar_refactoring.v1.patch
>
> http://www.postgresql.org/message-id/53868E57.3030908@dalibo.com
>
> Is any reason or is acceptable incompatible change CONNECTION_LIMIT
> instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
> for breaking compatibility?
>
> Surely this patch cannot be backported what is proposed there.
>
> Regards
>
> Pavel
>
>
>

On 06/21/2014 10:11 PM, Pavel Stehule wrote:
> Hello
>
> I am looking createdb_alterdb_grammar_refactoring.v1.patch
>
> http://www.postgresql.org/message-id/53868E57.3030908@dalibo.com

Thank you for looking at this.

> Is any reason or is acceptable incompatible change CONNECTION_LIMIT
> instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
> for breaking compatibility?

How is compatibility broken? The grammar still accepts the old way, I
just changed the documentation to promote the new way.

> Surely this patch cannot be backported what is proposed there.

There are reasons I can think of not to backport this first patch, but
breaking compatibility isn't one of them.
--
Vik

On 06/21/2014 10:21 PM, Pavel Stehule wrote:
> Second question related to second patch:
>
> Must be new syntax ALLOW_CONNECTIONS?

It doesn't *have* to be called that, but that's what the corresponding
column in pg_database is called so why add confusion? (Actually, it's
called datallowconn but that would be a silly name on the SQL level.)

> Should not be (ENABLE | DISABLE) CONNECTION ?

I don't think it should be, no.

> This doesn't need any new keyword.

None of this requires any new keywords. That's the whole point of the
refactoring patch.
--
Vik

2014-06-21 23:14 GMT+02:00 Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>:

> On 06/21/2014 10:11 PM, Pavel Stehule wrote:
> > Hello
> >
> > I am looking createdb_alterdb_grammar_refactoring.v1.patch
> >
> > http://www.postgresql.org/message-id/53868E57.3030908@dalibo.com
>
> Thank you for looking at this.
>
> > Is any reason or is acceptable incompatible change CONNECTION_LIMIT
> > instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
> > for breaking compatibility?
>
> How is compatibility broken? The grammar still accepts the old way, I
> just changed the documentation to promote the new way.
>
> > Surely this patch cannot be backported what is proposed there.
>
> There are reasons I can think of not to backport this first patch, but
> breaking compatibility isn't one of them.
>

I am sorry, tomorrow I have to read it again

Pavel

> --
> Vik
>

Hello

I returned to review this patch after sleeping - and I have to say, these
patches doesn't break a compatibility.

This feature has two patches:
createdb_alterdb_grammar_refactoring.v1-1.patch and
database_attributes.v2-1.patch. First patch do some cleaning in gram rules
a CREATE DATABASE and ALTER DATABASE statements (and introduce a
CONNECTION_LIMIT property). Second patch introduces ALLOW_CONNECTIONS and
IS_TEMPLATE database properties. A motivation for these patches is cleaning
alterdb/createdb grammars and drop necessity to directly modify pg_database
table.

1. these patch does what was proposed, there was not any objection in
related discussion
2. I can apply these patches cleanly, a compilation was without new
warnings and without errors
3. all tests was passed
4. there is a necessary documentation (for new features)
5. a new syntax is actively used in initdb and pg_upgrade. I am not sure,
if some special test is necessary and if we are able to test it.

Refactoring of alterdb/createdb grammars has sense and we would it.

I found only one problem - first patch introduce a new property
CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
documentation. But "CONNECTION LIMIT" is still supported, but it is not
documented. So for some readers it can look like breaking compatibility,
but it is false. This should be documented better.

Regards

Pavel

2014-06-21 23:14 GMT+02:00 Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>:

> On 06/21/2014 10:11 PM, Pavel Stehule wrote:
> > Hello
> >
> > I am looking createdb_alterdb_grammar_refactoring.v1.patch
> >
> > http://www.postgresql.org/message-id/53868E57.3030908@dalibo.com
>
> Thank you for looking at this.
>
> > Is any reason or is acceptable incompatible change CONNECTION_LIMIT
> > instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
> > for breaking compatibility?
>
> How is compatibility broken? The grammar still accepts the old way, I
> just changed the documentation to promote the new way.
>
> > Surely this patch cannot be backported what is proposed there.
>
> There are reasons I can think of not to backport this first patch, but
> breaking compatibility isn't one of them.
> --
> Vik
>

On Sun, Jun 22, 2014 at 2:59 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I found only one problem - first patch introduce a new property
> CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
> documentation. But "CONNECTION LIMIT" is still supported, but it is not
> documented. So for some readers it can look like breaking compatibility, but
> it is false. This should be documented better.

Yeah, I think the old syntax should be documented also. See, e.g.,
what we do for COPY.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/23/2014 06:21 PM, Robert Haas wrote:
> On Sun, Jun 22, 2014 at 2:59 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I found only one problem - first patch introduce a new property
>> CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
>> documentation. But "CONNECTION LIMIT" is still supported, but it is not
>> documented. So for some readers it can look like breaking compatibility, but
>> it is false. This should be documented better.
>
> Yeah, I think the old syntax should be documented also.

Why do we want to document syntax that should eventually be deprecated?

> See, e.g., what we do for COPY.

Exactly. We're still carrying around baggage from 7.2!

Backward compatibility: yes.
Backward documentation: no.
--
Vik

2014-06-23 18:39 GMT+02:00 Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>:

> On 06/23/2014 06:21 PM, Robert Haas wrote:
> > On Sun, Jun 22, 2014 at 2:59 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> >> I found only one problem - first patch introduce a new property
> >> CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
> >> documentation. But "CONNECTION LIMIT" is still supported, but it is not
> >> documented. So for some readers it can look like breaking
> compatibility, but
> >> it is false. This should be documented better.
> >
> > Yeah, I think the old syntax should be documented also.
>
> Why do we want to document syntax that should eventually be deprecated?
>

It is fair to our users. It can be deprecated, ok, we can write in doc -
this feature will be deprecated in next three years. Don't use it, but this
should be documentated.

Pavel

>
> > See, e.g., what we do for COPY.
>
> Exactly. We're still carrying around baggage from 7.2!
>
> Backward compatibility: yes.
> Backward documentation: no.
> --
> Vik
>

On Mon, Jun 23, 2014 at 12:39 PM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> wrote:
> On 06/23/2014 06:21 PM, Robert Haas wrote:
>> On Sun, Jun 22, 2014 at 2:59 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> I found only one problem - first patch introduce a new property
>>> CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
>>> documentation. But "CONNECTION LIMIT" is still supported, but it is not
>>> documented. So for some readers it can look like breaking compatibility, but
>>> it is false. This should be documented better.
>>
>> Yeah, I think the old syntax should be documented also.
>
> Why do we want to document syntax that should eventually be deprecated?

Because otherwise existing users will wonder if their dumps can still
be restored on newer systems.

And also, because documentation is, in general, a good thing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/23/2014 06:45 PM, Pavel Stehule wrote:
>
>
>
> 2014-06-23 18:39 GMT+02:00 Vik Fearing <vik(dot)fearing(at)dalibo(dot)com
> <mailto:vik(dot)fearing(at)dalibo(dot)com>>:
>
> On 06/23/2014 06:21 PM, Robert Haas wrote:
> > On Sun, Jun 22, 2014 at 2:59 PM, Pavel Stehule
> <pavel(dot)stehule(at)gmail(dot)com <mailto:pavel(dot)stehule(at)gmail(dot)com>> wrote:
> >> I found only one problem - first patch introduce a new property
> >> CONNECTION_LIMIT and replace previously used "CONNECTION LIMIT" in
> >> documentation. But "CONNECTION LIMIT" is still supported, but it
> is not
> >> documented. So for some readers it can look like breaking
> compatibility, but
> >> it is false. This should be documented better.
> >
> > Yeah, I think the old syntax should be documented also.
>
> Why do we want to document syntax that should eventually be deprecated?
>
>
> It is fair to our users. It can be deprecated, ok, we can write in doc -
> this feature will be deprecated in next three years. Don't use it, but
> this should be documentated.

Okay, here is version two of the refactoring patch that documents that
the with-space version is deprecated but still accepted.

The feature patch is not affected by this and so I am not attaching a
new version of that.
--
Vik

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
> On 06/21/2014 10:11 PM, Pavel Stehule wrote:
>> Is any reason or is acceptable incompatible change CONNECTION_LIMIT
>> instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
>> for breaking compatibility?

> How is compatibility broken? The grammar still accepts the old way, I
> just changed the documentation to promote the new way.

While I agree that this patch wouldn't break backwards compatibility,
I don't really see what the argument is for changing the recommended
spelling of the command.

The difficulty with doing what you've done here is that it creates
unnecessary cross-version incompatibilities; for example a 9.5 psql
being used against a 9.4 server would tab-complete the wrong spelling
of the option. Back-patching would change the set of versions for
which the problem exists, but it wouldn't remove the problem altogether.
And in fact it'd add new problems, e.g. pg_dumpall output from a 9.3.5
pg_dumpall failing to load into a 9.3.4 server. This is not the kind of
change we customarily back-patch anyway.

So personally I'd have just made connection_limit be an undocumented
internal equivalent for CONNECTION LIMIT, and kept the latter as the
preferred spelling, with no client-side changes.

regards, tom lane

2014-06-29 21:09 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
> > On 06/21/2014 10:11 PM, Pavel Stehule wrote:
> >> Is any reason or is acceptable incompatible change CONNECTION_LIMIT
> >> instead CONNECTION LIMIT? Is decreasing parser size about 1% good enough
> >> for breaking compatibility?
>
> > How is compatibility broken? The grammar still accepts the old way, I
> > just changed the documentation to promote the new way.
>
> While I agree that this patch wouldn't break backwards compatibility,
> I don't really see what the argument is for changing the recommended
> spelling of the command.
>
> The difficulty with doing what you've done here is that it creates
> unnecessary cross-version incompatibilities; for example a 9.5 psql
> being used against a 9.4 server would tab-complete the wrong spelling
> of the option. Back-patching would change the set of versions for
> which the problem exists, but it wouldn't remove the problem altogether.
> And in fact it'd add new problems, e.g. pg_dumpall output from a 9.3.5
> pg_dumpall failing to load into a 9.3.4 server. This is not the kind of
> change we customarily back-patch anyway.
>
> So personally I'd have just made connection_limit be an undocumented
> internal equivalent for CONNECTION LIMIT, and kept the latter as the
> preferred spelling, with no client-side changes.
>

+1

There is no important reason do hard changes in this moment

Pavel

>
> regards, tom lane
>

Vik Fearing <vik(dot)fearing(at)dalibo(dot)com> writes:
> Okay, here is version two of the refactoring patch that documents that
> the with-space version is deprecated but still accepted.

> The feature patch is not affected by this and so I am not attaching a
> new version of that.

I've committed this without the changes to expose the CONNECTION_LIMIT
spelling, and with some other minor fixes --- the only one of substance
being that you'd broken the "foo = DEFAULT" variants of the options by
removing the checks on whether defel->arg was provided.

regards, tom lane