Re: Redundant statements

Hi developers!

Column statistcs are lost in the re-engineered SQL in the SQL pane in
pgAdmin 1.10.2.
I have just filed ticket #160 in trac describing the problem.

Regards
Erwin

Le 26/03/2010 17:10, Erwin Brandstetter a écrit :
> [...]
> Column statistcs are lost in the re-engineered SQL in the SQL pane in
> pgAdmin 1.10.2.
> I have just filed ticket #160 in trac describing the problem.
>

Not sure this really is a bug. We don't put in the SQL pane of the table
all changes about the columns. People can still get that part by
clicking on the column node.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 26.03.2010 20:47, guillaume(at)lelarge(dot)info wrote:
> Le 26/03/2010 17:10, Erwin Brandstetter a écrit :
>
>> [...]
>> Column statistcs are lost in the re-engineered SQL in the SQL pane in
>> pgAdmin 1.10.2.
>> I have just filed ticket #160 in trac describing the problem.
>>
>>
> Not sure this really is a bug. We don't put in the SQL pane of the table
> all changes about the columns. People can still get that part by
> clicking on the column node.
>

My understanding up until now was, that the displayed SQL can be used to
recreate an identical table. But maybe that's where I am going wrong?

Regards
Erwin

Le 27/03/2010 01:07, Erwin Brandstetter a écrit :
> On 26.03.2010 20:47, guillaume(at)lelarge(dot)info wrote:
>> Le 26/03/2010 17:10, Erwin Brandstetter a écrit :
>>
>>> [...]
>>> Column statistcs are lost in the re-engineered SQL in the SQL pane in
>>> pgAdmin 1.10.2.
>>> I have just filed ticket #160 in trac describing the problem.
>>>
>>>
>> Not sure this really is a bug. We don't put in the SQL pane of the table
>> all changes about the columns. People can still get that part by
>> clicking on the column node.
>>
>
> My understanding up until now was, that the displayed SQL can be used to
> recreate an identical table. But maybe that's where I am going wrong?
>

I tried a few things and it seems you're right. For example, comments on
columns are available both in the Column SQL pane and in the Table SQL pane.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Le 27/03/2010 02:51, Guillaume Lelarge a écrit :
> Le 27/03/2010 01:07, Erwin Brandstetter a écrit :
>> On 26.03.2010 20:47, guillaume(at)lelarge(dot)info wrote:
>>> Le 26/03/2010 17:10, Erwin Brandstetter a écrit :
>>>
>>>> [...]
>>>> Column statistcs are lost in the re-engineered SQL in the SQL pane in
>>>> pgAdmin 1.10.2.
>>>> I have just filed ticket #160 in trac describing the problem.
>>>>
>>>>
>>> Not sure this really is a bug. We don't put in the SQL pane of the table
>>> all changes about the columns. People can still get that part by
>>> clicking on the column node.
>>>
>>
>> My understanding up until now was, that the displayed SQL can be used to
>> recreate an identical table. But maybe that's where I am going wrong?
>>
>
> I tried a few things and it seems you're right. For example, comments on
> columns are available both in the Column SQL pane and in the Table SQL pane.
>

OK, I have a patch (attached) that seems to work. It adds some
functions, and I'm not sure I should push this into the 1.10 branch.
What do you guys think about this? only in trunk or in trunk and in the
1.10 branch?

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On Sat, Apr 3, 2010 at 4:01 AM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> OK, I have a patch (attached) that seems to work. It adds some
> functions, and I'm not sure I should push this into the 1.10 branch.
> What do you guys think about this? only in trunk or in trunk and in the
> 1.10 branch?

Seems like a bug fix to me, so I say 1.10.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Le 03/04/2010 11:19, Dave Page a écrit :
> On Sat, Apr 3, 2010 at 4:01 AM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>> OK, I have a patch (attached) that seems to work. It adds some
>> functions, and I'm not sure I should push this into the 1.10 branch.
>> What do you guys think about this? only in trunk or in trunk and in the
>> 1.10 branch?
>
> Seems like a bug fix to me, so I say 1.10.
>

OK, commited to both branches.

Thanks.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Aloha!

I am testing Guillaume's version of pgadmin3.exe from Apr. 17.

The fix below is included and it seems to fix the problem just fine.
However, as a side effect (?) there is now a redundant statement for
each and every column:
ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
or
ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;

I think those statements should only be included, if the storage type
differs from the default setting - like "SET STATISTICS" is handled now.
It is pretty noisy as it is now.

On a side note: I would handle "WITH (OIDS=FALSE)" for table definitions
likewise: only print it, if it differs from the default setting.

Regards
Erwin

On 03.04.2010 11:46, guillaume(at)lelarge(dot)info wrote:
> Le 03/04/2010 11:19, Dave Page a écrit :
>
>> On Sat, Apr 3, 2010 at 4:01 AM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>
>>> OK, I have a patch (attached) that seems to work. It adds some
>>> functions, and I'm not sure I should push this into the 1.10 branch.
>>> What do you guys think about this? only in trunk or in trunk and in the
>>> 1.10 branch?
>>>
>> Seems like a bug fix to me, so I say 1.10.
>>
>>
> OK, commited to both branches.
>
> Thanks.
>

On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
> Aloha!
>
> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>
> The fix below is included and it seems to fix the problem just fine.
> However, as a side effect (?) there is now a redundant statement for
> each and every column:
> ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
> or
> ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>
> I think those statements should only be included, if the storage type
> differs from the default setting - like "SET STATISTICS" is handled now.
> It is pretty noisy as it is now.
>
> On a side note: I would handle "WITH (OIDS=FALSE)" for table
> definitions likewise: only print it, if it differs from the default
> setting.

Maybe an option "Show complete SQL"? To switch between "complete"
(noisy) and default display (only what is necessary to create an
identical object with the current settings).
Personally I am only interested in the "compact" version, but other
people's preferences may vary?

This would be a major wishlist item. Not complicated, but possibly many
lines of code.
What do you think of it? Should I create a ticket?

Regards
Erwin

Sorry for not answering sooner.

Le 29/04/2010 14:15, Erwin Brandstetter a écrit :
> On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
>> Aloha!
>>
>> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>>
>> The fix below is included and it seems to fix the problem just fine.
>> However, as a side effect (?) there is now a redundant statement for
>> each and every column:
>> ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
>> or
>> ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>>
>> I think those statements should only be included, if the storage type
>> differs from the default setting - like "SET STATISTICS" is handled now.
>> It is pretty noisy as it is now.

"SET STATISTICS" is not really handled this way. We had "SET STATISTICS"
statement if the value is bigger than zero. (Because if it is zero,
PostgreSQL will use the default_statistics_target setting.

Anyway, I agree that "SET STORAGE" is really noisy. Suppose a table with
20 columns. You will have 20 statements about storage. I agree that
something like this is needed. My problem is: what should we use as a
default value? PLAIN would be the default on non-toastable columns, and
EXTENDED the one to use with toastable columns? is there a way to be
sure of this?

>> On a side note: I would handle "WITH (OIDS=FALSE)" for table
>> definitions likewise: only print it, if it differs from the default
>> setting.

If we do this, a user won't be able to copy the SQL and paste/execute it
on another server if this one has another value for default_with_oids.
So, I'm against its removal (unless someone could prove I'm wrong :) ).

> Maybe an option "Show complete SQL"? To switch between "complete"
> (noisy) and default display (only what is necessary to create an
> identical object with the current settings).
> Personally I am only interested in the "compact" version, but other
> people's preferences may vary?

I'm not sure about this either. How could a user know about the
differences between a complete and a partial SQL?

> This would be a major wishlist item. Not complicated, but possibly many
> lines of code.

Well, actually, the only thing I think is needed here is to display (or
not) some column's properties. Not complicated, not a lot of code.

> What do you think of it? Should I create a ticket?

Well, I would like to have a better handling of the "SET STORAGE"
statement. So, yes, you can create a ticket on that, without being too
specific on the solution, which still needs some talk.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
> Sorry for not answering sooner.
>

No problem, the thread is old, but I just posted that yesterday. :)

> Le 29/04/2010 14:15, Erwin Brandstetter a écrit :
>
>> On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
>>
>>> Aloha!
>>>
>>> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>>>
>>> The fix below is included and it seems to fix the problem just fine.
>>> However, as a side effect (?) there is now a redundant statement for
>>> each and every column:
>>> ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
>>> or
>>> ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>>>
>>> I think those statements should only be included, if the storage type
>>> differs from the default setting - like "SET STATISTICS" is handled now.
>>> It is pretty noisy as it is now.
>>>
> "SET STATISTICS" is not really handled this way. We had "SET STATISTICS"
> statement if the value is bigger than zero. (Because if it is zero,
> PostgreSQL will use the default_statistics_target setting.
>

Actually, 0 (zero) disables statistics gathering. -1 is for reverting to
the system default.
http://www.postgresql.org/docs/8.4/interactive/sql-altertable.html

> Anyway, I agree that "SET STORAGE" is really noisy. Suppose a table with
> 20 columns. You will have 20 statements about storage. I agree that
> something like this is needed. My problem is: what should we use as a
> default value? PLAIN would be the default on non-toastable columns, and
> EXTENDED the one to use with toastable columns? is there a way to be
> sure of this?
>
>
>>> On a side note: I would handle "WITH (OIDS=FALSE)" for table
>>> definitions likewise: only print it, if it differs from the default
>>> setting.
>>>
> If we do this, a user won't be able to copy the SQL and paste/execute it
> on another server if this one has another value for default_with_oids.
> So, I'm against its removal (unless someone could prove I'm wrong :) ).
>

As it is now, the policy is somewhat unclear. Some elements are added
even though they are redundant under the local settings, others are not.
If we could switch between complete and compact display, this would help
to clear the situation.
In my daily work I reuse code in the same db cluster 99% of the time -
95% of the time in the same db. Redundant SQL is only cluttering the
display and slows me down on a regular basis.
One or the other redundant key word should not hurt, but we could strip
most of the noise. (Reduces traffic a bit, too.)
For cases where we want to copy objects to a different environment, the
option "complete SQL" should be an improvement, too.

I would define "local settings" as what "SHOW ALL" returns for the
current connection (database / user defaults are in effect).

>> Maybe an option "Show complete SQL"? To switch between "complete"
>> (noisy) and default display (only what is necessary to create an
>> identical object with the current settings).
>> Personally I am only interested in the "compact" version, but other
>> people's preferences may vary?
>>
> I'm not sure about this either. How could a user know about the
> differences between a complete and a partial SQL?
>

We could have a note at the very top (possibly optional., too):
-- SQL complete
-- SQL for current connection
Current settings are defined by the connection parameters displayed in
the toolbar: host, user, database.

>> This would be a major wishlist item. Not complicated, but possibly many
>> lines of code.
>>
> Well, actually, the only thing I think is needed here is to display (or
> not) some column's properties. Not complicated, not a lot of code.
>

Even better. :) But there are probably a number of cases, where we might
want to add SQL to the "complete" version or trim in the "compact" version.

>> What do you think of it? Should I create a ticket?
>>
> Well, I would like to have a better handling of the "SET STORAGE"
> statement. So, yes, you can create a ticket on that, without being too
> specific on the solution, which still needs some talk.
>

So we agree on "SET STORAGE". I'll create a ticket on that.

The option to switch between "complete" and "compact" is a different
(more fundamental) feature. I would like it a lot, but I am not coding,
so I can only suggest.

Regards
Erwin

Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>> Sorry for not answering sooner.
>>
>
> No problem, the thread is old, but I just posted that yesterday. :)
>
>
>> Le 29/04/2010 14:15, Erwin Brandstetter a écrit :
>>
>>> On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
>>>
>>>> Aloha!
>>>>
>>>> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>>>>
>>>> The fix below is included and it seems to fix the problem just fine.
>>>> However, as a side effect (?) there is now a redundant statement for
>>>> each and every column:
>>>> ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
>>>> or
>>>> ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>>>>
>>>> I think those statements should only be included, if the storage type
>>>> differs from the default setting - like "SET STATISTICS" is handled
>>>> now.
>>>> It is pretty noisy as it is now.
>>>>
>> "SET STATISTICS" is not really handled this way. We had "SET STATISTICS"
>> statement if the value is bigger than zero. (Because if it is zero,
>> PostgreSQL will use the default_statistics_target setting.
>>
>
> Actually, 0 (zero) disables statistics gathering. -1 is for reverting to
> the system default.
> http://www.postgresql.org/docs/8.4/interactive/sql-altertable.html
>

Sorry, was wrong on this one :)

>> Anyway, I agree that "SET STORAGE" is really noisy. Suppose a table with
>> 20 columns. You will have 20 statements about storage. I agree that
>> something like this is needed. My problem is: what should we use as a
>> default value? PLAIN would be the default on non-toastable columns, and
>> EXTENDED the one to use with toastable columns? is there a way to be
>> sure of this?
>>
>>
>>>> On a side note: I would handle "WITH (OIDS=FALSE)" for table
>>>> definitions likewise: only print it, if it differs from the default
>>>> setting.
>>>>
>> If we do this, a user won't be able to copy the SQL and paste/execute it
>> on another server if this one has another value for default_with_oids.
>> So, I'm against its removal (unless someone could prove I'm wrong :) ).
>>
>
> As it is now, the policy is somewhat unclear. Some elements are added
> even though they are redundant under the local settings, others are not.
> If we could switch between complete and compact display, this would help
> to clear the situation.

+1

> In my daily work I reuse code in the same db cluster 99% of the time -
> 95% of the time in the same db. Redundant SQL is only cluttering the
> display and slows me down on a regular basis.
> One or the other redundant key word should not hurt, but we could strip
> most of the noise. (Reduces traffic a bit, too.)
> For cases where we want to copy objects to a different environment, the
> option "complete SQL" should be an improvement, too.
>
> I would define "local settings" as what "SHOW ALL" returns for the
> current connection (database / user defaults are in effect).
>

OK.

>>> Maybe an option "Show complete SQL"? To switch between "complete"
>>> (noisy) and default display (only what is necessary to create an
>>> identical object with the current settings).
>>> Personally I am only interested in the "compact" version, but other
>>> people's preferences may vary?
>>>
>> I'm not sure about this either. How could a user know about the
>> differences between a complete and a partial SQL?
>>
>
> We could have a note at the very top (possibly optional., too):
> -- SQL complete
> -- SQL for current connection
> Current settings are defined by the connection parameters displayed in
> the toolbar: host, user, database.
>

Seems better to me.

>>> This would be a major wishlist item. Not complicated, but possibly many
>>> lines of code.
>>>
>> Well, actually, the only thing I think is needed here is to display (or
>> not) some column's properties. Not complicated, not a lot of code.
>>
>
> Even better. :) But there are probably a number of cases, where we might
> want to add SQL to the "complete" version or trim in the "compact" version.
>

Sure. We'll need complete details on these cases :)

>>> What do you think of it? Should I create a ticket?
>>>
>> Well, I would like to have a better handling of the "SET STORAGE"
>> statement. So, yes, you can create a ticket on that, without being too
>> specific on the solution, which still needs some talk.
>>
>
> So we agree on "SET STORAGE". I'll create a ticket on that.
>

Yeah. I also found how to detect the "initial" value of the storage
(column typstorage in pg_catalog.pg_type). So I'll work on this right now.

> The option to switch between "complete" and "compact" is a different
> (more fundamental) feature. I would like it a lot, but I am not coding,
> so I can only suggest.
>

You can also create a ticket on this. But I'll refrain working on this,
waiting for Dave's opinion on this issue. And it probably won't be for
1.12, as beta will begin in a few days.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

Le 29/04/2010 17:50, Guillaume Lelarge a écrit :
> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
> [...]
>>>> What do you think of it? Should I create a ticket?
>>>>
>>> Well, I would like to have a better handling of the "SET STORAGE"
>>> statement. So, yes, you can create a ticket on that, without being too
>>> specific on the solution, which still needs some talk.
>>>
>>
>> So we agree on "SET STORAGE". I'll create a ticket on that.
>>
>
> Yeah. I also found how to detect the "initial" value of the storage
> (column typstorage in pg_catalog.pg_type). So I'll work on this right now.
>

Fixed. I can provide you a new pgAdmin3.exe quite easily if you want.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 29.04.2010 18:48, guillaume(at)lelarge(dot)info wrote:
> Le 29/04/2010 17:50, Guillaume Lelarge a écrit :
>
>> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>>
>>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>>>
>> [...]
>>
>>>>> What do you think of it? Should I create a ticket?
>>>>>
>>>>>
>>>> Well, I would like to have a better handling of the "SET STORAGE"
>>>> statement. So, yes, you can create a ticket on that, without being too
>>>> specific on the solution, which still needs some talk.
>>>>
>>>>
>>> So we agree on "SET STORAGE". I'll create a ticket on that.
>>>
>>>
>> Yeah. I also found how to detect the "initial" value of the storage
>> (column typstorage in pg_catalog.pg_type). So I'll work on this right now.
>>
>>
> Fixed. I can provide you a new pgAdmin3.exe quite easily if you want.
>

Cool. If you provide me with a new pgAdmin3.exe I will use it for tests.
We are moving in on a point release v1.10.3, aren't we? I found 21
resolved bugs tagged "1.10.3" in trac - compared to 9 for v1.10.2. So we
might call it early alpha-testing?

Regards
Erwin

Le 30/04/2010 00:35, Erwin Brandstetter a écrit :
> On 29.04.2010 18:48, guillaume(at)lelarge(dot)info wrote:
>> Le 29/04/2010 17:50, Guillaume Lelarge a écrit :
>>
>>> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>>>
>>>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>>>>
>>> [...]
>>>
>>>>>> What do you think of it? Should I create a ticket?
>>>>>>
>>>>>>
>>>>> Well, I would like to have a better handling of the "SET STORAGE"
>>>>> statement. So, yes, you can create a ticket on that, without being too
>>>>> specific on the solution, which still needs some talk.
>>>>>
>>>>>
>>>> So we agree on "SET STORAGE". I'll create a ticket on that.
>>>>
>>>>
>>> Yeah. I also found how to detect the "initial" value of the storage
>>> (column typstorage in pg_catalog.pg_type). So I'll work on this right
>>> now.
>>>
>>>
>> Fixed. I can provide you a new pgAdmin3.exe quite easily if you want.
>>
>
> Cool. If you provide me with a new pgAdmin3.exe I will use it for tests.

On it.

> We are moving in on a point release v1.10.3, aren't we? I found 21
> resolved bugs tagged "1.10.3" in trac - compared to 9 for v1.10.2. So we
> might call it early alpha-testing?
>

Not right now. Dave is working on getting out beta 1 for pgAdmin 1.12. I
don't think he has the time to prepare a new 1.10 minor release, and I'm
not sure this is the perfect timing for it. But I hope we'll have one
before 1.12 is final.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 30.04.2010 00:40, guillaume(at)lelarge(dot)info wrote:
> Le 30/04/2010 00:35, Erwin Brandstetter a écrit :
>
>> On 29.04.2010 18:48, guillaume(at)lelarge(dot)info wrote:
>>
>>> Le 29/04/2010 17:50, Guillaume Lelarge a écrit :
>>>
>>>
>>>> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>>>>
>>>>
>>>>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>>>>>
>>>>>
>>>> [...]
>>>>
>>>>
>>>>>>> What do you think of it? Should I create a ticket?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Well, I would like to have a better handling of the "SET STORAGE"
>>>>>> statement. So, yes, you can create a ticket on that, without being too
>>>>>> specific on the solution, which still needs some talk.
>>>>>>
>>>>>>
>>>>>>
>>>>> So we agree on "SET STORAGE". I'll create a ticket on that.
>>>>>
>>>>>
>>>>>
>>>> Yeah. I also found how to detect the "initial" value of the storage
>>>> (column typstorage in pg_catalog.pg_type). So I'll work on this right
>>>> now.
>>>>
>>>>
>>>>
>>> Fixed. I can provide you a new pgAdmin3.exe quite easily if you want.
>>>
>>>
>> Cool. If you provide me with a new pgAdmin3.exe I will use it for tests.
>>
> On it.
>
>
>> We are moving in on a point release v1.10.3, aren't we? I found 21
>> resolved bugs tagged "1.10.3" in trac - compared to 9 for v1.10.2. So we
>> might call it early alpha-testing?
>>
>>
> Not right now. Dave is working on getting out beta 1 for pgAdmin 1.12. I
> don't think he has the time to prepare a new 1.10 minor release, and I'm
> not sure this is the perfect timing for it. But I hope we'll have one
> before 1.12 is final.
>

Either way, fine with me. As a bugfix release, 1.10.3 wouldn't need a
lot of testing, I guess.

Good night!
Erwin

On Thu, Apr 29, 2010 at 11:51 PM, Erwin Brandstetter
<brandstetter(at)falter(dot)at> wrote:
>> Not right now. Dave is working on getting out beta 1 for pgAdmin 1.12. I
>> don't think he has the time to prepare a new 1.10 minor release, and I'm
>> not sure this is the perfect timing for it. But I hope we'll have one
>> before 1.12 is final.
>>
>
> Either way, fine with me. As a bugfix release, 1.10.3 wouldn't need a lot of
> testing, I guess.

Normally I wouldn't bother as we're so close to 1.12, but we're using
1.10 in our upcoming Postgres Plus Advanced Server release, so I do
expect I'll cut a 1.10.3 soon.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

Hi Dave!

I send a reply on the thread to you, Dave. Guillaume seems to like the
idea somewhat. I wonder what you think about it.

On 29.04.2010 17:50, guillaume(at)lelarge(dot)info wrote:
> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>
>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>>
>>> Le 29/04/2010 14:15, Erwin Brandstetter a écrit :
>>>
>>>
>>>> On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
>>>>
>>>>
>>>>> Aloha!
>>>>>
>>>>> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>>>>>
>>>>> The fix below is included and it seems to fix the problem just fine.
>>>>> However, as a side effect (?) there is now a redundant statement for
>>>>> each and every column:
>>>>> ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
>>>>> or
>>>>> ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>>>>>
>>>>> I think those statements should only be included, if the storage type
>>>>> differs from the default setting - like "SET STATISTICS" is handled
>>>>> now.
>>>>> It is pretty noisy as it is now.
>>>>>
>>>>>
>>> (...)
>>>>> On a side note: I would handle "WITH (OIDS=FALSE)" for table
>>>>> definitions likewise: only print it, if it differs from the default
>>>>> setting.
>>>>>
>>>>>
>>> If we do this, a user won't be able to copy the SQL and paste/execute it
>>> on another server if this one has another value for default_with_oids.
>>> So, I'm against its removal (unless someone could prove I'm wrong :) ).
>>>
>>>
>> As it is now, the policy is somewhat unclear. Some elements are added
>> even though they are redundant under the local settings, others are not.
>> If we could switch between complete and compact display, this would help
>> to clear the situation.
>>
> +1
>
>
>> In my daily work I reuse code in the same db cluster 99% of the time -
>> 95% of the time in the same db. Redundant SQL is only cluttering the
>> display and slows me down on a regular basis.
>> One or the other redundant key word should not hurt, but we could strip
>> most of the noise. (Reduces traffic a bit, too.)
>> For cases where we want to copy objects to a different environment, the
>> option "complete SQL" should be an improvement, too.
>>
>> I would define "local settings" as what "SHOW ALL" returns for the
>> current connection (database / user defaults are in effect).
>>
> OK.
>
>
>>>> Maybe an option "Show complete SQL"? To switch between "complete"
>>>> (noisy) and default display (only what is necessary to create an
>>>> identical object with the current settings).
>>>> Personally I am only interested in the "compact" version, but other
>>>> people's preferences may vary?
>>>>
>>>>
>>> I'm not sure about this either. How could a user know about the
>>> differences between a complete and a partial SQL?
>>>
>>>
>> We could have a note at the very top (possibly optional., too):
>> -- SQL complete
>> -- SQL for current connection
>> Current settings are defined by the connection parameters displayed in
>> the toolbar: host, user, database.
>>
>>
> Seems better to me.
>

In pg 9.0 we might also want to take default ACLs for the database /
schema into consideration. But this can be optional.
In a first approach we could provide a full set of GRANT / REVOKE
statements in both verions. Trim the "compact" version according to
default ACLs later.
It might be an additional source for the definition of the "current"
connection, though - besides host, database and user.

>>>> This would be a major wishlist item. Not complicated, but possibly many
>>>> lines of code.
>>>>
>>>>
>>> Well, actually, the only thing I think is needed here is to display (or
>>> not) some column's properties. Not complicated, not a lot of code.
>>>
>>>
>> Even better. :) But there are probably a number of cases, where we might
>> want to add SQL to the "complete" version or trim in the "compact" version.
>>
>>
> Sure. We'll need complete details on these cases :)
>

We could start by cloning the status quo, and then add statements /
clauses to the complete version and trim redundant stuff from the
compact version step by step. This way the kickoff would be simple.

> (...)
>> The option to switch between "complete" and "compact" is a different
>> (more fundamental) feature. I would like it a lot, but I am not coding,
>> so I can only suggest.
>>
>>
> You can also create a ticket on this. But I'll refrain working on this,
> waiting for Dave's opinion on this issue. And it probably won't be for
> 1.12, as beta will begin in a few days.

OK, most likely not for pgAdmin 1.12. Maybe for the next release? Next
point release if the changes to the code base are not too invasive?

So, what do you think of the idea, Dave? I would love the "compact"
version, while I would be happy about the "complete" version on
occasions, too.
Depending on your answer I would add a writeup of the feature as a
wishlist item in trac.

Regards
Erwin

Sorry - missed that. I generally prefer to only include SQL DDL for
things that are non-default.

On Fri, May 7, 2010 at 6:43 PM, Erwin Brandstetter
<brandstetter(at)falter(dot)at> wrote:
> Hi Dave!
>
> I send a reply on the thread to you, Dave. Guillaume seems to like the idea
> somewhat. I wonder what you think about it.
>
>
> On 29.04.2010 17:50, guillaume(at)lelarge(dot)info wrote:
>>
>> Le 29/04/2010 17:28, Erwin Brandstetter a écrit :
>>
>>>
>>> On 29.04.2010 15:21, guillaume(at)lelarge(dot)info wrote:
>>>
>>>>
>>>> Le 29/04/2010 14:15, Erwin Brandstetter a écrit :
>>>>
>>>>
>>>>>
>>>>> On 28.04.2010 19:16, brandstetter(at)falter(dot)at wrote:
>>>>>
>>>>>
>>>>>>
>>>>>> Aloha!
>>>>>>
>>>>>> I am testing Guillaume's version of pgadmin3.exe from Apr. 17.
>>>>>>
>>>>>> The fix below is included and it seems to fix the problem just fine.
>>>>>> However, as a side effect (?) there is now a redundant statement for
>>>>>> each and every column:
>>>>>>      ALTER TABLE foo ALTER COLUMN bar SET STORAGE PLAIN;
>>>>>> or
>>>>>>      ALTER TABLE foo ALTER COLUMN baz SET STORAGE EXTENDED;
>>>>>>
>>>>>> I think those statements should only be included, if the storage type
>>>>>> differs from the default setting - like "SET STATISTICS" is handled
>>>>>> now.
>>>>>> It is pretty noisy as it is now.
>>>>>>
>>>>>>
>>>>
>>>> (...)
>>>>>>
>>>>>> On a side note: I would handle "WITH (OIDS=FALSE)" for table
>>>>>> definitions likewise: only print it, if it differs from the default
>>>>>> setting.
>>>>>>
>>>>>>
>>>>
>>>> If we do this, a user won't be able to copy the SQL and paste/execute it
>>>> on another server if this one has another value for default_with_oids.
>>>> So, I'm against its removal (unless someone could prove I'm wrong :) ).
>>>>
>>>>
>>>
>>> As it is now, the policy is somewhat unclear. Some elements are added
>>> even though they are redundant under the local settings, others are not.
>>> If we could switch between complete and compact display, this would help
>>> to clear the situation.
>>>
>>
>> +1
>>
>>
>>>
>>> In my daily work I reuse code in the same db cluster 99% of the time -
>>> 95% of the time in the same db. Redundant SQL is only cluttering the
>>> display and slows me down on a regular basis.
>>> One or the other redundant key word should not hurt, but we could strip
>>> most of the noise. (Reduces traffic a bit, too.)
>>> For cases where we want to copy objects to a different environment, the
>>> option "complete SQL" should be an improvement, too.
>>>
>>> I would define "local settings" as what "SHOW ALL" returns for the
>>> current connection (database / user defaults are in effect).
>>>
>>
>> OK.
>>
>>
>>>>>
>>>>> Maybe an option "Show complete SQL"? To switch between "complete"
>>>>> (noisy) and default display (only what is necessary to create an
>>>>> identical object with the current settings).
>>>>> Personally I am only interested in the "compact" version, but other
>>>>> people's preferences may vary?
>>>>>
>>>>>
>>>>
>>>> I'm not sure about this either. How could a user know about the
>>>> differences between a complete and a partial SQL?
>>>>
>>>>
>>>
>>> We could have a note at the very top (possibly optional., too):
>>> -- SQL complete
>>> -- SQL for current connection
>>> Current settings are defined by the connection parameters displayed in
>>> the toolbar: host, user, database.
>>>
>>>
>>
>> Seems better to me.
>>
>
> In pg 9.0 we might also want to take default ACLs for the database / schema
> into consideration. But this can be optional.
> In a first approach we could provide a full set of GRANT / REVOKE statements
> in both verions. Trim the "compact" version according to default ACLs later.
> It might be an additional source for the definition of the "current"
> connection, though - besides host, database and user.
>
>
>>>>> This would be a major wishlist item. Not complicated, but possibly many
>>>>> lines of code.
>>>>>
>>>>>
>>>>
>>>> Well, actually, the only thing I think is needed here is to display (or
>>>> not) some column's properties. Not complicated, not a lot of code.
>>>>
>>>>
>>>
>>> Even better. :) But there are probably a number of cases, where we might
>>> want to add SQL to the "complete" version or trim in the "compact"
>>> version.
>>>
>>>
>>
>> Sure. We'll need complete details on these cases :)
>>
>
> We could start by cloning the status quo, and then add statements / clauses
> to the complete version and trim redundant stuff from the compact version
> step by step. This way the kickoff would be simple.
>
>
>> (...)
>>>
>>> The option to switch between "complete" and "compact" is a different
>>> (more fundamental) feature. I would like it a lot, but I am not coding,
>>> so I can only suggest.
>>>
>>>
>>
>> You can also create a ticket on this. But I'll refrain working on this,
>> waiting for Dave's opinion on this issue. And it probably won't be for
>> 1.12, as beta will begin in a few days.
>
> OK, most likely not for pgAdmin 1.12. Maybe for the next release? Next point
> release if the changes to the code base are not too invasive?
>
> So, what do you think of the idea, Dave? I would love the "compact" version,
> while I would be happy about the "complete" version on occasions, too.
> Depending on your answer I would add a writeup of the feature as a wishlist
> item in trac.
>
>
> Regards
> Erwin
>
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 07.05.2010 21:21, dpage(at)pgadmin(dot)org wrote:
> Sorry - missed that. I generally prefer to only include SQL DDL for
> things that are non-default.

I generally agree. I see the "complete" variant as an option. The
"compact" (non-default SQL DDL) version is what would make my work easier.
However, at the time being we have a mixture. How would you define
"non-default"?

Regards
Erwin

On Mon, May 10, 2010 at 3:20 PM, Erwin Brandstetter
<brandstetter(at)falter(dot)at> wrote:
> On 07.05.2010 21:21, dpage(at)pgadmin(dot)org wrote:
>>
>> Sorry - missed that. I generally prefer to only include SQL DDL for
>> things that are non-default.
>
> I generally agree. I see the "complete" variant as an option. The "compact"
> (non-default SQL DDL) version is what would make my work easier.
> However, at the time being we have a mixture. How would you define
> "non-default"?

Anything where explicit DDL is required to recreate the object as it
is. If the DDL is redundant (ie. it tries to set the value we get if
we don't use it at all), then it should be omitted.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 10.05.2010 21:16, dpage(at)pgadmin(dot)org wrote:
> On Mon, May 10, 2010 at 3:20 PM, Erwin Brandstetter
> <brandstetter(at)falter(dot)at> wrote:
>
>> On 07.05.2010 21:21, dpage(at)pgadmin(dot)org wrote:
>>
>>> Sorry - missed that. I generally prefer to only include SQL DDL for
>>> things that are non-default.
>>>
>> I generally agree. I see the "complete" variant as an option. The "compact"
>> (non-default SQL DDL) version is what would make my work easier.
>> However, at the time being we have a mixture. How would you define
>> "non-default"?
>>
> Anything where explicit DDL is required to recreate the object as it
> is. If the DDL is redundant (ie. it tries to set the value we get if
> we don't use it at all), then it should be omitted.
>

That's what we have been discussing. I would define "default" for the
current connection as what "SHOW ALL" returns.
Default settings of a connection are defined by host, user, database
(same parameters as displayed in the toolbar of the SQL window):
Plus, possibly default ACLs for the database / schema in pg 9.0. (Did i
get all sources?)

As it is now, there are many redundant statements / clauses included.
Examples:

CREATE TABLE foo
(
foo_id serial NOT NULL,
note text,
...
)
WITH (
OIDS=FALSE -- redundant, as OIDS=FALSE is the cluster-wide
default.
)

ALTER TABLE foo OWNER TO postgres; -- redundant for the current user
postgres

CONSTRAINT ort_land_id_fk FOREIGN KEY (land_id)
REFERENCES land (land_id) MATCH SIMPLE -- "MATCH
SIMPLE" is default
ON UPDATE CASCADE ON DELETE NO ACTION,

CREATE INDEX ort_ort_idx
ON ort
USING btree -- redundant as it is default
(ort);

etc.

On the other hand, if somebody wants to copy objects from one connection
to another (different host, user, ...) "complete" SQL DDL would be
pretty useful.

Regards
Erwin