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@sss.pgh.pa.us> wrote:
> > Robert Haas <robertmhaas@gmail.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.