Alvaro,
* Alvaro Herrera (alvherre@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