Re: Small fix for _equalValue() - Mailing list pgsql-patches

Tom Lane wrote:
>
> Fernando Nasser <fnasser@redhat.com> writes:
> > *************** _equalValue(Value *a, Value *b)
> > *** 1771,1777 ****
> >               case T_Float:
> >               case T_String:
> >               case T_BitString:
> > !                     return strcmp(a->val.str, b->val.str) == 0;
> >               default:
> >                       break;
> >       }
> > --- 1771,1780 ----
> >               case T_Float:
> >               case T_String:
> >               case T_BitString:
> > !                     if ((a->val.str != NULL) && (b->val.str != NULL))
> > !                             return strcmp(a->val.str, b->val.str) == 0;
> > !                     else
> > !                             return a->val.ival == b->val.ival;  /* true if both are NULL */
> >               default:
> >                       break;
> >       }
>
> Several comments here:
>
> This is not the idiomatic way to do it; there is an equalstr() macro
> in equalfuncs.c that does this pushup for you.  So "return
> equalstr(a->val.str, b->val.str)" would be the appropriate fix.
>

I see.  Thanks, I will fix the patch and resubmit it (see below).


> Possibly a more interesting question, though, is *why* equalValue is
> seeing Values with null pointer parts.  I cannot think of any good
> reason to consider that a legal data structure.  Do you know where this
> construct is coming from?  I'd be inclined to consider the source at
> fault, not equalValue.
>

Someone is using NULL strings in gram.y, like in:

VariableSetStmt:  SET ColId TO var_value
                {
                    VariableSetStmt *n = makeNode(VariableSetStmt);
                    n->name  = $2;
                    n->args = makeList1(makeStringConst($4, NULL));
                    $$ = (Node *) n;
                }

there are several instances of it, all related to variable set.

Well, NULL is a valid value for a (char *) so this seems legal
enough to me.

I still think we should handle NULL pointer values in equalValue.
(we can through an ERROR if we decide to disallow NULL pointers in
Value -- we must go after who added it to VariableSet or revert that
change though).



> On the other fixes: as a rule, a field-typing bug in copyfuncs suggests
> an equivalent bug over in equalfuncs, and vice versa; as well as
> possible errors in readfuncs/outfuncs.  Did you look?
>

Yes, but I will double check all the same.

Thanks for the comments.

--
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9