Thread

Re: [v9.2] DROP Reworks Part.0 - 'missing_ok' support of get_object_address

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2011-06-19T08:48:03Z

Thanks for your review.

2011/6/19 Robert Haas <robertmhaas@gmail.com>:
> On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>> The attached patch is a preparation to rework implementation of DROP statement
>> using a common code. That intends to apply get_object_address() to resolve name
>> of objects to be removed, and eventually minimizes the number of places to put
>> permission checks.
>>
>> Its first step is an enhancement of get_object_address; to accept 'missing_ok'
>> argument to handle cases when IF EXISTS clause is supplied.
>> If 'missing_ok' was true and the supplied name was not found, the patched
>> get_object_address() returns an ObjectAddress with InvalidOid as objectId.
>> If 'missing_ok' was false, its behavior is not changed.
>
> Let's consistently make missing_ok the last argument to all of the
> functions to which we're adding it.
>
OK. I revised position of the 'missing_ok' argument.

> I don't think it's a good idea for get_relation_by_qualified_name() to
> be second-guessing the error message that RangeVarGetRelid() feels
> like throwing.
>
OK. I revised the patch to provide 'true' on RangeVarGetRelid().
Its side effect is on error messages when user gives undefined object name.

> I think that attempting to fetch the column foo.bar when foo doesn't
> exist should be an error even if missing_ok is true.  I believe that's
> consistent with what we do elsewhere.  (If it *were* necessary to open
> the relation without failing if it's not there, you could use
> try_relation_openrv instead of doing as you've done here.)
>
It was fixed. AlterTable() already open the relation (without missing_ok)
in the case when we drop a column, so I don't think we need to acquire
relation locks if the supplied column was missing.

> There is certainly a more compact way of writing the logic in
> get_object_address_typeobj.  Also, I think that function should be
> called get_object_address_type(); the "obj" on the end seems
> redundant.
>
I renamed the function name to get_object_address_type(), and
consolidate initialization of ObjectAddress variables.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>