Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > I'm confused. Why does ExecBuildSlotValueDescription() return an empty > string instead of NULL? There doesn't seem to be any value left in that > idea, and returning NULL would make the callsites slightly simpler as > well. (Also, I think the comment on top of it should be updated to > reflect the permissions-related issues.)
The comment on top of ExecBuildSlotValueDescription() does include this: * Note that, like BuildIndexValueDescription, if the user does not have * permission to view any of the columns involved, an empty string is returned. Is that insufficient? > It seems that the reason for this is to be consistent with > BuildIndexValueDescription, which seems to do the same thing to simplify > the stuff going on at check_exclusion_constraint() -- by returning an > empty string, that code doesn't need to check which of the returned > values is empty, only whether both are. That seems an odd choice to me; > it seems better to me to be specific, i.e. include each of the %s > depending on whether the returned string is null or not. You would have > three possible different errdetails, but that seems a good thing anyway. Right, ExecBuildSlotValueDescription() was made to be consistent with BuildIndexValueDescription. The reason for BuildIndexValueDescription returning an empty string is different from why you hypothosize though- it's exported and I was a bit worried that existing external callers might not be prepared for it to return a NULL instead of a string of some kind. If this was a green field instead of a back-patch fix, I'd certainly return NULL instead. If others feel that's not a good reason to avoid returning NULL, I can certainly change it around. > (Also, ISTM the "if (!strlen(foo))" idiom should be avoided because it > is confusing; better test explicitely for zero or nonzero. Anyway if > you change the functions to return NULL, you can test for NULL-ness > easily and there's no possible confusion anymore.) Yeah, I thought I had eliminated all of those on my own review, but looks like I missed one. Updated patch attached. Thanks! Stephen
signature.asc
Description: Digital signature