Joe Conway <mail@joeconway.com> writes:
> Well, maybe the attached patches better explain what I mean.
> In the case of the 8.2 patch, a very small code change allows new
> regression data including NULL rowids to:
> 1) not crash
> 2) have no impact otherwise
> The much bigger 8.3 patch shows that for the very same new regression
> data, there is a significant impact on the output (i.e. NULL rowids get
> their own output row as discussed).
> I'm still leaning toward applying the 8.2 patch for back branches but
> I'll bow to the general consensus.
I'd vote for the bigger patch all the way back. The smaller patch has
nothing to recommend it except being smaller. It replaces the crash
with a behavior that will change in 8.3, thus creating a potential
portability issue for users of (post-repair) back branches. Why not
get it right the first time?
A couple of minor thoughts:
* You could reduce the ugliness of many of the tests by introducing a
variant strcmp function that does the "right" things with NULL inputs.
It might also be worth adding a variant pstrdup that takes a NULL.
* Surely this bit:
> xpfree(lastrowid);
> ! if (rowid)
> ! lastrowid = pstrdup(rowid);
> }
needs to be:
if (rowid)
lastrowid = pstrdup(rowid);
else
lastrowid = NULL;
no? (Again the variant pstrdup would save some notation)
regards, tom lane
PS: I hear things are pretty crazy out your way -- hope the fire's
not too close to you.