On Tue, Jan 3, 2017 at 6:40 PM, Fabrízio de Royes Mello <fabriziome...@gmail.com> wrote: > 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, ...).
Right. There's comment.sql but it's about comments in language and not comments on database objects. It looks like the COMMENT ON for various objects is tested in test files for those objects and there isn't any tests testing databases e.g. ALTER DATABASE in sql directory. > Should we need also add test cases for this kind of > objects? > Possibly, we don't have those testcases, because those will affect existing objects when run through "make installcheck". But current database is always going to be "regression" for these tests. That database is dropped when installcheck starts. So, we can add a testcase for it. But I am not sure where should we add it. Creating a new file just for COMMENT ON DATABASE doesn't look good. > >> 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??? > Please read my reply to Robert's mail. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
