Skip site navigation (1) Skip section navigation (2)

Re: [GENERAL] Crosstab Problems

From: Joe Conway <mail(at)joeconway(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Jorge Godoy <jgodoy(at)gmail(dot)com>, scott(dot)marlowe(at)gmail(dot)com, Stefan Schwarzer <stefan(dot)schwarzer(at)grid(dot)unep(dot)ch>, regmeplease(at)gmail(dot)com, "Patches (PostgreSQL)" <pgsql-patches(at)postgresql(dot)org>
Subject: Re: [GENERAL] Crosstab Problems
Date: 2007-10-26 04:40:04
Message-ID: 47216FA4.4080905@joeconway.com (view raw or flat)
Thread:
Lists: pgsql-generalpgsql-patches
Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)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

In response to

Responses

pgsql-patches by date

Next:From: Joe ConwayDate: 2007-10-26 05:45:20
Subject: Re: [GENERAL] Crosstab Problems
Previous:From: Tom LaneDate: 2007-10-26 01:43:48
Subject: Re: [GENERAL] Crosstab Problems

pgsql-general by date

Next:From: Patrick TJ McPheeDate: 2007-10-26 04:40:35
Subject: Re: Selecting K random rows - efficiently!
Previous:From: Gowrishankar LDate: 2007-10-26 04:19:51
Subject: Data cube in PostgreSQL

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group