Thread: Small fix for _equalValue()
Avoid problems when one of the pointer values is NULL (or both).
_equalVariableSetStmt() dumps core without this one.
--
Fernando Nasser
Red Hat Canada Ltd. E-Mail: fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.114
diff -c -p -r1.114 equalfuncs.c
*** src/backend/nodes/equalfuncs.c 2002/03/06 20:34:48 1.114
--- src/backend/nodes/equalfuncs.c 2002/03/07 12:39:13
*************** _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;
}
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
Fernando Nasser <fnasser@redhat.com> writes:
> Tom Lane wrote:
>> 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.
> Someone is using NULL strings in gram.y, like in:
Ah, and the DEFAULT case returns a NULL.
IMHO this gram.y code is the broken part, not copyfuncs/equalfuncs.
There isn't any reason to build a Value with a null pointer --- and
there are probably a lot more places that will crash on one than just
copyfuncs/equalfuncs.
I note that SET DEFAULT was not done that way in 7.1, which IIRC was
the last time we had COPY_PARSE_PLAN_TREES on by default during a
development cycle. Might be time to turn it on again for awhile ;-).
(The reason we don't keep it on always is that that case can mask
bugs too. I like to flip the default setting every few months, but
I think I forgot to do it anytime during the 7.2 cycle.)
regards, tom lane
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.
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.
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?
regards, tom lane
Tom Lane wrote:
>
> 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.
>
Here it is. Thanks again.
--
Fernando Nasser
Red Hat Canada Ltd. E-Mail: fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario M4P 2C9Index: src/backend/nodes/equalfuncs.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v
retrieving revision 1.114
diff -c -p -r1.114 equalfuncs.c
*** src/backend/nodes/equalfuncs.c 2002/03/06 20:34:48 1.114
--- src/backend/nodes/equalfuncs.c 2002/03/07 15:43:28
*************** _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,1777 ----
case T_Float:
case T_String:
case T_BitString:
! return equalstr(a->val.str, b->val.str);
default:
break;
}
Thomas Lockhart <lockhart@fourpalms.org> writes:
> If this should be done differently I'm happy for suggestions...
I think DEFAULT should probably be represented by a NULL, not by
a Value node containing a null string pointer.
I'm willing to do the work if no one else feels strongly about it ;-)
regards, tom lane
...
> Someone is using NULL strings in gram.y, like in:
> n->args = makeList1(makeStringConst($4, NULL));
> 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.
Ah, that was me, to allow comma-delimited lists of parameters to be sent
to the SET handling code. In previous versions, multi-parameter
arguments had to be enclosed in quotes. For most of the SET variables,
lists aren't indicated, but I wanted to use the list for all cases to
minimize the special cases downstream.
If this should be done differently I'm happy for suggestions...
- Thomas
> > If this should be done differently I'm happy for suggestions...
> I think DEFAULT should probably be represented by a NULL, not by
> a Value node containing a null string pointer.
> I'm willing to do the work if no one else feels strongly about it ;-)
OK. I can't think of a case where we would want to represent multiple
DEFAULT placeholders in the context of SET.
Or if we are going to pick up on the recent proposal to allow
column-specific DEFAULT values perhaps we should use a common
representation for the solution here?
In either case, I won't feel stepped on if you implement the solution,
but I can do so if desired.
- Thomas
Thomas Lockhart <lockhart@fourpalms.org> writes:
> Or if we are going to pick up on the recent proposal to allow
> column-specific DEFAULT values perhaps we should use a common
> representation for the solution here?
Huh? I recall someone working to allow expressions for type-specific
default values, but I didn't think anything was happening for columns.
> In either case, I won't feel stepped on if you implement the solution,
> but I can do so if desired.
Your choice ...
regards, tom lane
> > 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:
...
> 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.
OK, I've committed a patch to the stable and development trees which
adds a guard check in three places in gram.y, matching the guards
already in place for other cass in the same area.
Fernando, can you please check this (preferably without your patches to
guard the output functions, since those mask the upstream problem)?
Or, can you give me the complete test case you are using to demonstrate
the problem to make sure I'm testing the thing you are seeing?
While I'm looking at it, the "SET key=val" area is somewhat "kludge on
kludge" to enable lists of values, at least partly because it was at the
end of the development cycle and I didn't want to ripple breakage into
parts of the code I wasn't trying to touch. I'll go through and try to
rationalize it sometime soon (actually, I've already started but haven't
finished).
- Thomas
Thomas Lockhart <thomas@fourpalms.org> writes:
> Fernando, can you please check this (preferably without your patches to
> guard the output functions, since those mask the upstream problem)?
> Or, can you give me the complete test case you are using to demonstrate
> the problem to make sure I'm testing the thing you are seeing?
I believe the test case is just to compile tcop/postgres.c with
COPY_PARSE_PLAN_TREES #defined, and see if the regression tests pass...
regards, tom lane
> I believe the test case is just to compile tcop/postgres.c with
> COPY_PARSE_PLAN_TREES #defined, and see if the regression tests pass...
OK, that works, afaik without having or requiring Fernando's patches.
I'll plan on going through the parser code a bit more in the next week
or so.
- Thomas
Thomas Lockhart <thomas@fourpalms.org> writes:
>> I believe the test case is just to compile tcop/postgres.c with
>> COPY_PARSE_PLAN_TREES #defined, and see if the regression tests pass...
> OK, that works, afaik without having or requiring Fernando's patches.
That's because I already committed the other changes he pointed out ;-).
But yeah, we seem to be copy-clean again.
regards, tom lane