From: | Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)com> |
---|---|
To: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com> |
Cc: | Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Subject: | Re: Add support to COMMENT ON CURRENT DATABASE |
Date: | 2017-01-03 13:10:03 |
Message-ID: | CAFcNs+oq-GrHLnvbSU8NBuyzdbHDwDdSuU+0fM9=91GYdjrJCg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Ashutosh,
First of all thanks for your review.
On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>
> The patch has white space error
> git apply /mnt/hgfs/tmp/comment_on_current_database_v1.patch
> /mnt/hgfs/tmp/comment_on_current_database_v1.patch:52: trailing
whitespace.
> * schema-qualified or catalog-qualified.
> warning: 1 line adds whitespace errors.
>
I'll fix.
> The patch compiles clean, regression is clean. I tested auto
> completion of current database, as well pg_dumpall output for comments
> on multiple databases. Those are working fine. Do we want to add a
> testcase for testing the SQL functionality as well as pg_dumpall
> functionality?
>
While looking into the src/test/regress/sql files I didn't find any test
cases for shared objects (databases, roles, tablespaces, procedural
languages, ...). Should we need also add test cases for this kind of
objects?
> Instead of changing get_object_address_unqualified(),
> get_object_address_unqualified() and pg_get_object_address(), should
> we just stick get_database_name(MyDatabaseId) as object name in
> gram.y? That would be much cleaner than special handling of NIL list.
> It looks dubious to set that list as NIL when the target is some
> object. If we do that, we will spend extra cycles in finding database
> id which is ready available as MyDatabaseId, but the code will be
> cleaner. Another possibility is to use objnames to store the database
> name and objargs to store the database id. That means we overload
> objargs, which doesn't looks good either.
>
In the previous thread Alvaro point me out about a possible DDL deparse
inconsistency [1] and because this reason we decide to think better and
rework this patch.
I confess I'm not too happy with this code yet, and thinking better maybe
we should create a new object type called OBJECT_CURRENT_DATABASE to handle
it different than OBJECT_DATABASE. Opinions???
[1]
https://www.postgresql.org/message-id/20150429170406.GI4369%40alvh.no-ip.org
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2017-01-03 13:11:03 | Re: increasing the default WAL segment size |
Previous Message | Amit Kapila | 2017-01-03 12:59:21 | Re: Supporting huge pages on Windows |