Re: [GENERAL] Crosstab Problems - Mailing list pgsql-patches

Tom Lane wrote:
> 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?

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