Re: [HACKERS] Add support to COMMENT ON CURRENT DATABASE

On Tue, Jan 3, 2017 at 10:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jan 3, 2017 at 12:06 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.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