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

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