Re: Add support to COMMENT ON CURRENT DATABASE

From: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
To: Fabrízio Mello <fabriziomello(at)gmail(dot)com>
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add support to COMMENT ON CURRENT DATABASE
Date: 2017-01-03 05:06:42
Message-ID: CAFjFpRdppHTASs9YgLSD7T5RawvYS1w3Cjm8LNENhg09KUVT7A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

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?

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.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ashutosh Bapat 2017-01-03 05:30:47 Re: safer node casting
Previous Message Craig Ringer 2017-01-03 04:36:27 Re: [PATCH] PostgresNode.pm enhancements, pg_lsn helper, and some more recovery tests