Hello
2014/1/22 Dean Rasheed <dean.a.rash...@gmail.com> > 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 though about it too. But I didn't continue - reasons was named by Dean - and RemoveObjects are not difficult code - lot of code is mechanical - and it is not on critical path. Regards Pavel > > 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 >
