Re: Add support to COMMENT ON CURRENT DATABASE

On Tue, Jan 3, 2017 at 10:09 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> 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?
>
> No. Note this comment at the top of gram.y:
>
> * In general, nothing in this file should initiate database accesses
> * nor depend on changeable state (such as SET variables). If you do
> * database accesses, your code will fail when we have aborted the
> * current transaction and are just parsing commands to find the next
> * ROLLBACK or COMMIT. If you make use of SET variables, then you
> * will do the wrong thing in multi-query strings like this:
> * SET constraint_exclusion TO off; SELECT * FROM foo;
> * because the entire string is parsed by gram.y before the SET gets
> * executed. Anything that depends on the database or changeable state
> * should be handled during parse analysis so that it happens at the
> * right time not the wrong time.
>
> I grant you that MyDatabaseId can't (currently, anyway) change during
> the lifetime of a single backend, but it still seems like a bad idea
> to make gram.y depend on that. If nothing else, it's problematic if
> we want to deparse the DDL statement (as Fabrízio also points out).
>

Thanks for pointing that out.

I think that handling NIL list in get_object_address_unqualified(),
get_object_address_unqualified() and pg_get_object_address() doesn't
really look good. Intuitively having a NIL list indicates no object
being specified, hence those checks.

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