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

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.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.

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.

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.)

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.

Apart from those comments this looks pretty sensible.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for your review.

2011/6/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

Sorry, the previous revision did not update regression test part
towards the latest one.

2011/6/19 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> Thanks for your review.
>
> 2011/6/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Tue, Jun 14, 2011 at 8:06 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jun 19, 2011 at 7:40 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> Sorry, the previous revision did not update regression test part
> towards the latest one.

Some of the refactoring you've done here seems likely to break things,
because you're basically making the relation locking happen later than
it does not, and that's going to cause problems.
get_object_address_relobject() is a particularly egregious
rearrangement. It seems to me that the right formula is to call
relation_openrv() if missing_ok is false, and try_relation_openrv() if
missing_ok is true. But that's sort of a pain, so I propose to first
apply the attached patch, which gets rid of try_relation_openrv() and
try_heap_openrv() and instead adds a missing_ok argument to
relation_openrv() and heap_openrv(). If we do this, then the
missing_ok argument can just be passed through all the way down.

Thoughts? Comments? Objections?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Some of the refactoring you've done here seems likely to break things,
> because you're basically making the relation locking happen later than
> it does not, and that's going to cause problems.
> get_object_address_relobject() is a particularly egregious
> rearrangement. It seems to me that the right formula is to call
> relation_openrv() if missing_ok is false, and try_relation_openrv() if
> missing_ok is true. But that's sort of a pain, so I propose to first
> apply the attached patch, which gets rid of try_relation_openrv() and
> try_heap_openrv() and instead adds a missing_ok argument to
> relation_openrv() and heap_openrv(). If we do this, then the
> missing_ok argument can just be passed through all the way down.

> Thoughts? Comments? Objections?

At least the last hunk (in pltcl.c) seems to have the flag backwards.

regards, tom lane

On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Some of the refactoring you've done here seems likely to break things,
>> because you're basically making the relation locking happen later than
>> it does not, and that's going to cause problems.
>> get_object_address_relobject() is a particularly egregious
>> rearrangement.  It seems to me that the right formula is to call
>> relation_openrv() if missing_ok is false, and try_relation_openrv() if
>> missing_ok is true.  But that's sort of a pain, so I propose to first
>> apply the attached patch, which gets rid of try_relation_openrv() and
>> try_heap_openrv() and instead adds a missing_ok argument to
>> relation_openrv() and heap_openrv().  If we do this, then the
>> missing_ok argument can just be passed through all the way down.
>
>> Thoughts?  Comments?  Objections?
>
> At least the last hunk (in pltcl.c) seems to have the flag backwards.

Ah, nuts. Sorry. I think that and parse_relation.c are the only
places where the try variants are used; nobody else is willing to
fail, and everyone else is passing false.

Revised patch attached.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 21, 2011 at 11:11:41PM -0400, Robert Haas wrote:
> On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> Some of the refactoring you've done here seems likely to break things,
> >> because you're basically making the relation locking happen later than
> >> it does not, and that's going to cause problems.
> >> get_object_address_relobject() is a particularly egregious
> >> rearrangement. ?It seems to me that the right formula is to call
> >> relation_openrv() if missing_ok is false, and try_relation_openrv() if
> >> missing_ok is true. ?But that's sort of a pain, so I propose to first
> >> apply the attached patch, which gets rid of try_relation_openrv() and
> >> try_heap_openrv() and instead adds a missing_ok argument to
> >> relation_openrv() and heap_openrv(). ?If we do this, then the
> >> missing_ok argument can just be passed through all the way down.
> >
> >> Thoughts? ?Comments? ?Objections?
> >
> > At least the last hunk (in pltcl.c) seems to have the flag backwards.
>
> Ah, nuts. Sorry. I think that and parse_relation.c are the only
> places where the try variants are used; nobody else is willing to
> fail, and everyone else is passing false.
>
> Revised patch attached.

All existing call sites updated by this patch hardcode the flag, and only 3?
proposed call sites would take advantage of the ability to not do so. The
try_relation_openrv() case is fairly rare and likely to remain that way. Given
that, I mildly prefer the code as it is, even if that means doing "missing_ok ?
try_relation_openrv() : relation_openrv()" in a few places. Could always wrap
that in a static function of objectaddress.c.

On Wed, Jun 22, 2011 at 6:18 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Tue, Jun 21, 2011 at 11:11:41PM -0400, Robert Haas wrote:
>> On Tue, Jun 21, 2011 at 11:04 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> >> Some of the refactoring you've done here seems likely to break things,
>> >> because you're basically making the relation locking happen later than
>> >> it does not, and that's going to cause problems.
>> >> get_object_address_relobject() is a particularly egregious
>> >> rearrangement. ?It seems to me that the right formula is to call
>> >> relation_openrv() if missing_ok is false, and try_relation_openrv() if
>> >> missing_ok is true. ?But that's sort of a pain, so I propose to first
>> >> apply the attached patch, which gets rid of try_relation_openrv() and
>> >> try_heap_openrv() and instead adds a missing_ok argument to
>> >> relation_openrv() and heap_openrv(). ?If we do this, then the
>> >> missing_ok argument can just be passed through all the way down.
>> >
>> >> Thoughts? ?Comments? ?Objections?
>> >
>> > At least the last hunk (in pltcl.c) seems to have the flag backwards.
>>
>> Ah, nuts.  Sorry.  I think that and parse_relation.c are the only
>> places where the try variants are used; nobody else is willing to
>> fail, and everyone else is passing false.
>>
>> Revised patch attached.
>
> All existing call sites updated by this patch hardcode the flag, and only 3?
> proposed call sites would take advantage of the ability to not do so.  The
> try_relation_openrv() case is fairly rare and likely to remain that way.  Given
> that, I mildly prefer the code as it is, even if that means doing "missing_ok ?
> try_relation_openrv() : relation_openrv()" in a few places.  Could always wrap
> that in a static function of objectaddress.c.

I don't really like the idea of having a static function in
objectaddress.c, because I think there will eventually be more callers
who sometimes want to pass missing_ok = true and other times pass
missing_ok = false. Plus, it seems a little nutty to have a function
that, depending on whether its argument is true or false, calls one of
two other functions which are virtually cut-and-paste copies of each
other, except that one internally has true and the other false. I
just work here, though.

Another option might be to leave heap_openrv() and relation_openrv()
alone and add a missing_ok argument to try_heap_openrv() and
try_relation_openrv(). Passing true would give the same behavior as
presently; passing false would make them behave like the non-try
version.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Another option might be to leave heap_openrv() and relation_openrv()
> alone and add a missing_ok argument to try_heap_openrv() and
> try_relation_openrv().

+1 for that, although the try_ prefix might be inappropriate naming
now; how about cond_relation_openrv?

regards, tom lane

Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011:

> Another option might be to leave heap_openrv() and relation_openrv()
> alone and add a missing_ok argument to try_heap_openrv() and
> try_relation_openrv(). Passing true would give the same behavior as
> presently; passing false would make them behave like the non-try
> version.

That would be pretty weird, having two functions, one of them sometimes
doing the same thing as the other one.

I understand Noah's concern but I think your original proposal was saner
than both options presented so far.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011:
>
>> Another option might be to leave heap_openrv() and relation_openrv()
>> alone and add a missing_ok argument to try_heap_openrv() and
>> try_relation_openrv().  Passing true would give the same behavior as
>> presently; passing false would make them behave like the non-try
>> version.
>
> That would be pretty weird, having two functions, one of them sometimes
> doing the same thing as the other one.
>
> I understand Noah's concern but I think your original proposal was saner
> than both options presented so far.

I agree with you. If we had a whole pile of options it might be worth
having heap_openrv() and heap_openrv_extended() so as not to
complicate the simple case, but since there's no forseeable need to
add anything other than missing_ok, my gut is to just add it and call
it good.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I revised my patch based on your "there-is-no-try-v2.patch".
It enabled to implement 'missing_ok' support without modification of
orders to solve the object name and relation locking.

Thanks,

2011/6/22 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011:
>>
>>> Another option might be to leave heap_openrv() and relation_openrv()
>>> alone and add a missing_ok argument to try_heap_openrv() and
>>> try_relation_openrv().  Passing true would give the same behavior as
>>> presently; passing false would make them behave like the non-try
>>> version.
>>
>> That would be pretty weird, having two functions, one of them sometimes
>> doing the same thing as the other one.
>>
>> I understand Noah's concern but I think your original proposal was saner
>> than both options presented so far.
>
> I agree with you.  If we had a whole pile of options it might be worth
> having heap_openrv() and heap_openrv_extended() so as not to
> complicate the simple case, but since there's no forseeable need to
> add anything other than missing_ok, my gut is to just add it and call
> it good.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Jun 22, 2011 at 12:51 PM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Excerpts from Robert Haas's message of mié jun 22 08:56:02 -0400 2011:
>>
>>> Another option might be to leave heap_openrv() and relation_openrv()
>>> alone and add a missing_ok argument to try_heap_openrv() and
>>> try_relation_openrv().  Passing true would give the same behavior as
>>> presently; passing false would make them behave like the non-try
>>> version.
>>
>> That would be pretty weird, having two functions, one of them sometimes
>> doing the same thing as the other one.
>>
>> I understand Noah's concern but I think your original proposal was saner
>> than both options presented so far.
>
> I agree with you.  If we had a whole pile of options it might be worth
> having heap_openrv() and heap_openrv_extended() so as not to
> complicate the simple case, but since there's no forseeable need to
> add anything other than missing_ok, my gut is to just add it and call
> it good.

On further review, my gut is having second thoughts. This patch is an
awful lot smaller and easier to verify correctness if I just mess with
the "try" calls and not the regular ones; and it avoids both
back-patching hazards for us and hoops for third-party loadable
modules that are using the non-try versions of those functions to jump
through.

Third try attached...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote:
> On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > I agree with you. ?If we had a whole pile of options it might be worth
> > having heap_openrv() and heap_openrv_extended() so as not to
> > complicate the simple case, but since there's no forseeable need to
> > add anything other than missing_ok, my gut is to just add it and call
> > it good.
>
> On further review, my gut is having second thoughts. This patch is an
> awful lot smaller and easier to verify correctness if I just mess with
> the "try" calls and not the regular ones; and it avoids both
> back-patching hazards for us and hoops for third-party loadable
> modules that are using the non-try versions of those functions to jump
> through.

+1. (Note that the function header comments need a few more updates.)

On Mon, Jun 27, 2011 at 2:59 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote:
>> On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > I agree with you. ?If we had a whole pile of options it might be worth
>> > having heap_openrv() and heap_openrv_extended() so as not to
>> > complicate the simple case, but since there's no forseeable need to
>> > add anything other than missing_ok, my gut is to just add it and call
>> > it good.
>>
>> On further review, my gut is having second thoughts.  This patch is an
>> awful lot smaller and easier to verify correctness if I just mess with
>> the "try" calls and not the regular ones; and it avoids both
>> back-patching hazards for us and hoops for third-party loadable
>> modules that are using the non-try versions of those functions to jump
>> through.
>
> +1.  (Note that the function header comments need a few more updates.)

Oh, good catch, thanks. Committed with some further comment changes.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

The attached patch is rebased one towards the latest tree, using
relation_openrv_extended().

Although it is not a matter in this patch itself, I found a problem on
the upcoming patch
that consolidate routines associated with DropStmt.
Existing RemoveRelations() acquires a lock on the table owning an
index to be removed
in the case when OBJECT_INDEX is supplied.
However, the revised get_object_address() opens the supplied relation
(= index) in same
time with lookup of its name. So, we may break down the
relation_openrv_extended()
into a pair of RangeVarGetRelid() and relation_open().

Any good idea?

2011/6/27 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Jun 27, 2011 at 2:59 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> On Mon, Jun 27, 2011 at 01:28:30PM -0400, Robert Haas wrote:
>>> On Wed, Jun 22, 2011 at 1:36 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> > I agree with you. ?If we had a whole pile of options it might be worth
>>> > having heap_openrv() and heap_openrv_extended() so as not to
>>> > complicate the simple case, but since there's no forseeable need to
>>> > add anything other than missing_ok, my gut is to just add it and call
>>> > it good.
>>>
>>> On further review, my gut is having second thoughts.  This patch is an
>>> awful lot smaller and easier to verify correctness if I just mess with
>>> the "try" calls and not the regular ones; and it avoids both
>>> back-patching hazards for us and hoops for third-party loadable
>>> modules that are using the non-try versions of those functions to jump
>>> through.
>>
>> +1.  (Note that the function header comments need a few more updates.)
>
> Oh, good catch, thanks.  Committed with some further comment changes.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Jun 27, 2011 at 4:40 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is rebased one towards the latest tree, using
> relation_openrv_extended().

Committed.

> Although it is not a matter in this patch itself, I found a problem on
> the upcoming patch
> that consolidate routines associated with DropStmt.
> Existing RemoveRelations() acquires a lock on the table owning an
> index to be removed
> in the case when OBJECT_INDEX is supplied.
> However, the revised get_object_address() opens the supplied relation
> (= index) in same
> time with lookup of its name. So, we may break down the
> relation_openrv_extended()
> into a pair of RangeVarGetRelid() and relation_open().

Not without looking at the patch. I will respond on that thread when
I've read through it more thoroughly.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company