Hi Ashutosh, First of all thanks for your review.
On Tue, Jan 3, 2017 at 3:06 AM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.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
