On 21 January 2014 22:28, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> I have been mulling over this patch, and I can't seem to come to terms
> with it. I first started making it look nicer here and there, thinking
> it was all mostly okay, but eventually arrived at the idea that it seems
> wrong to do what this does: basically, get_object_address() tries to
> obtain an object address, and if that fails, return InvalidOid; then, in
> RemoveObjects, we try rather hard to figure out why that failed, and
> construct an error message.
>
> It seems to me that it would make more sense to have get_object_address
> somehow return a code indicating what failed; then we don't have to go
> all over through the parser code once more. Perhaps, for example, when
> missing_ok is given as true to get_object_address it also needs to get a
> pointer to ObjectType and a string; if some object does not exist then
> fill the ObjectType with the failing object and the string with the
> failing name. Then RemoveObjects can construct a string more easily.
> Not sure how workable this exact idea is; maybe there is a better way.
>
Yeah, when I initially started reviewing this patch I had a very
similar thought. But when I looked more deeply at the code beneath
get_object_address, I started to doubt whether it could be done
without rather extensive changes all over the place. Also
get_object_address is itself called from a lot of places (not
necessarily all in our code) and all the other places (in our code, at
least) pass missing_ok=false. So it seemed rather ugly to change its
signature and force a matching change in all those other places, which
actually don't care about missing objects. Perhaps the answer would be
to have a separate get_object_address_if_exists function, and remove
the missing_ok flag from get_object_address, but that all felt like a
much larger patch.
In the end, I felt that Pavel's approach wasn't adding that much new
code, and it's all localised in the one place that does actually
tolerate missing objects.
I admit though, that I didn't explore the other approach very deeply,
so perhaps it might fall out more neatly than I feared.
Regards,
Dean