Tom Lane wrote:
Joe Conway <[EMAIL PROTECTED]> 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?

OK, I can live with that.

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.

I had thoughts along those lines -- it would certainly make the code more readable. I'll go ahead and do that but it won't be in time for a 26 October beta2.

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

Well I had already defined xpfree like this:
8<------------------
#define xpfree(var_) \
        do { \
                if (var_ != NULL) \
                { \
                        pfree(var_); \
                        var_ = NULL; \
                } \
        } while (0)
8<------------------
so lastrowid is already NULL (I sometimes wish this was the default behavior for pfree() itself). But the point about pstrdup variant is well taken, and I guess the xpfree behavior is not obvious, so it deserves at least a comment.

                        regards, tom lane

PS: I hear things are pretty crazy out your way -- hope the fire's
not too close to you.

We packed and were ready to evacuate two or three times, but never actually had to leave our house, thankfully. The closest the fire ever got was about 4 miles, and at this point I don't think we're in any more direct danger. But I know many people who were not so fortunate :-(

Joe

---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
      choose an index scan if your joining column's datatypes do not
      match

Reply via email to