Re: Comments on SQL/Med objects

Hi,

While working on adding support for SQL/Med objects to pgAdmin, I'm
quite surprised to see there is no way to add comments to SQL/Med
objects. Is this on purpose or is it just something that was simply missed?

Thanks.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Tue, Mar 22, 2011 at 6:23 PM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> While working on adding support for SQL/Med objects to pgAdmin, I'm
> quite surprised to see there is no way to add comments to SQL/Med
> objects. Is this on purpose or is it just something that was simply missed?

I think it's an oversight. We should probably fix this.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Mar 22, 2011 at 6:23 PM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
>> While working on adding support for SQL/Med objects to pgAdmin, I'm
>> quite surprised to see there is no way to add comments to SQL/Med
>> objects. Is this on purpose or is it just something that was simply missed?

> I think it's an oversight. We should probably fix this.

Yeah, I had a private TODO about that. I'd like to see if we can
refactor the grammar to eliminate some of the duplication there
as well as the potential for oversights of this sort. I believe
that USER MAPPINGs are missing from ObjectType as well as a bunch
of other basic places ...

regards, tom lane

Le 23/03/2011 17:53, Tom Lane a écrit :
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Mar 22, 2011 at 6:23 PM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>> While working on adding support for SQL/Med objects to pgAdmin, I'm
>>> quite surprised to see there is no way to add comments to SQL/Med
>>> objects. Is this on purpose or is it just something that was simply missed?
>
>> I think it's an oversight. We should probably fix this.
>
> Yeah, I had a private TODO about that. I'd like to see if we can
> refactor the grammar to eliminate some of the duplication there
> as well as the potential for oversights of this sort. I believe
> that USER MAPPINGs are missing from ObjectType as well as a bunch
> of other basic places ...
>

OK, great. Thanks for your answers.

--
Guillaume
http://www.postgresql.fr
http://dalibo.com

On Wed, Mar 23, 2011 at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Mar 22, 2011 at 6:23 PM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>>> While working on adding support for SQL/Med objects to pgAdmin, I'm
>>> quite surprised to see there is no way to add comments to SQL/Med
>>> objects. Is this on purpose or is it just something that was simply missed?
>
>> I think it's an oversight.  We should probably fix this.
>
> Yeah, I had a private TODO about that.  I'd like to see if we can
> refactor the grammar to eliminate some of the duplication there
> as well as the potential for oversights of this sort.  I believe
> that USER MAPPINGs are missing from ObjectType as well as a bunch
> of other basic places ...

Are you going to work on this? If not I can pick it up, at least
insofar as making the comment stuff work across the board.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Mar 23, 2011 at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Yeah, I had a private TODO about that. I'd like to see if we can
>> refactor the grammar to eliminate some of the duplication there
>> as well as the potential for oversights of this sort. I believe
>> that USER MAPPINGs are missing from ObjectType as well as a bunch
>> of other basic places ...

> Are you going to work on this? If not I can pick it up, at least
> insofar as making the comment stuff work across the board.

I'm still up to my rear in collations, so feel free.

regards, tom lane

On Mon, Mar 28, 2011 at 12:06 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Mar 23, 2011 at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Yeah, I had a private TODO about that.  I'd like to see if we can
>>> refactor the grammar to eliminate some of the duplication there
>>> as well as the potential for oversights of this sort.  I believe
>>> that USER MAPPINGs are missing from ObjectType as well as a bunch
>>> of other basic places ...
>
>> Are you going to work on this?  If not I can pick it up, at least
>> insofar as making the comment stuff work across the board.
>
> I'm still up to my rear in collations, so feel free.

OK. I'll work on it this week.

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

On Mon, Mar 28, 2011 at 12:29 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Mar 28, 2011 at 12:06 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Wed, Mar 23, 2011 at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> Yeah, I had a private TODO about that.  I'd like to see if we can
>>>> refactor the grammar to eliminate some of the duplication there
>>>> as well as the potential for oversights of this sort.  I believe
>>>> that USER MAPPINGs are missing from ObjectType as well as a bunch
>>>> of other basic places ...
>>
>>> Are you going to work on this?  If not I can pick it up, at least
>>> insofar as making the comment stuff work across the board.
>>
>> I'm still up to my rear in collations, so feel free.
>
> OK.  I'll work on it this week.

Attached. Foreign tables are already OK, I believe; it's only foreign
data wrappers and foreign servers that appear to need fixing.

The fact that foreign data wrapper is sometimes abbreviated to fdw and
sometimes not does nothing for the greppability of the code. I'm
wondering if we should go through and fix the constants that
abbreviate it:

ACL_KIND_FDW
ACL_ALL_RIGHTS_FDW
OBJECT_FDW
OCLASS_FDW

It seems to me that it would be a whole lot clearer and easier if
these all spelled it out FOREIGN_DATA_WRAPPER, as we do for similar
object types. Other than a pretty minute back-patch hazard, I don't
see much down side. Thoughts?

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

On Thu, 31 Mar 2011 11:24:27 -0400
Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Attached. Foreign tables are already OK, I believe; it's only foreign
> data wrappers and foreign servers that appear to need fixing.

The patch seems good for basic functionarity. I've tested the patch
and noticed that get_foreign_data_wrapper_oid() is same as
GetForeignDataWrapperOidByName(), so they could be merged. Also
GetForeignServerOidByName() could be merged.

I changed "foreign data wrapper" in message to "foreign-data wrapper"
for consistency, but it's revertable.

Please see merge_oid_funcs.patch which can be applied onto your patch.

I think some supports can be added for comments on SQL/MED objects.

- pg_dump support for comment on fdw and server
- psql describe commands (\dew+ and \des+)
- psql TAB completion

Please see attached patches for each feature.

While testing pg_dump, I noticed that comment of extension's member
objects are not dumped by pg_dump. Those comments should be dumped
after CREATE EXTENSION statement?

Regards,
--
Shigeru Hanada

On Fri, Apr 1, 2011 at 9:40 AM, Shigeru HANADA
<hanada(at)metrosystems(dot)co(dot)jp> wrote:
> On Thu, 31 Mar 2011 11:24:27 -0400
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Attached.  Foreign tables are already OK, I believe; it's only foreign
>> data wrappers and foreign servers that appear to need fixing.
>
> The patch seems good for basic functionarity.  I've tested the patch
> and noticed that get_foreign_data_wrapper_oid() is same as
> GetForeignDataWrapperOidByName(), so they could be merged.  Also
> GetForeignServerOidByName() could be merged.
>
> I changed "foreign data wrapper" in message to "foreign-data wrapper"
> for consistency, but it's revertable.
>
> Please see merge_oid_funcs.patch which can be applied onto your patch.

Thanks for the review, good catches. Committed those two patches
together with a bit of further rearrangement.

> I think some supports can be added for comments on SQL/MED objects.
>
>  - pg_dump support for comment on fdw and server
>  - psql describe commands (\dew+ and \des+)
>  - psql TAB completion
>
> Please see attached patches for each feature.

I'll take a look at these next.

> While testing pg_dump, I noticed that comment of extension's member
> objects are not dumped by pg_dump.  Those comments should be dumped
> after CREATE EXTENSION statement?

No, I don't believe that would be correct.

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

On Fri, Apr 1, 2011 at 9:40 AM, Shigeru HANADA
<hanada(at)metrosystems(dot)co(dot)jp> wrote:
>  - pg_dump support for comment on fdw and server

Applied, good catch, thanks.

>  - psql describe commands (\dew+ and \des+)

Not sure if we want this behavior change or not. Any other opinions?
It doesn't look like there's any particular consistency in terms of
which backslash commands include a description always (e.g. \dT),
which ones include it only when + is specified (e.g. \dt), and which
don't include it at all (e.g. \dc).

>  - psql TAB completion

Committed this also.

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