Small fix for _equalValue()

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:

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

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 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

...

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

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

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

...

That's because I already committed the other changes he pointed out ;-).
But yeah, we seem to be copy-clean again.

I had thought that you objected to the guard code in the copy functions
since nodes should not have had the content they did. And afaik I have
now fixed the upstream problems with the content.

Had you changed you mind about the necessity for the guard code? Why did
those patches get applied if the only feedback in the thread was that
the problem did not lie there?

Or are we talking about two different parts of the patch submission? I'm
a bit confused as to the current state of the code tree...

- Thomas

Thomas Lockhart <thomas@fourpalms.org> writes:

That's because I already committed the other changes he pointed out ;-).
But yeah, we seem to be copy-clean again.

I had thought that you objected to the guard code in the copy functions
since nodes should not have had the content they did. And afaik I have
now fixed the upstream problems with the content.

Right, the SET DEFAULT problem is fixed that way. Fernando had pointed
out a couple of problems in unrelated constructs (GRANT and something
else I forget now) that also needed to be fixed. Those fixes did get
committed.

Had you changed you mind about the necessity for the guard code?

No. I think Value is fine as-is.

regards, tom lane

No. I think Value is fine as-is.

Ah, great.

- Thomas