On 21 January 2014 22:28, Alvaro Herrera <alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
