Re: [pgsql-patches] [HACKERS] [PATCHES] Last infomask bit

2007-01-11 Thread Heikki Linnakangas

Tom Lane wrote:

Bruce Momjian [EMAIL PROTECTED] writes:

Tom Lane wrote:

Has anyone bothered to measure the overhead added by having to mask to
fetch or store the natts value?  This is not a zero-cost improvement.



Tom, how should this be tested?  I assume some loop of the same query
over and over again.


I'd be satisfied by a demonstration of no meaningful difference in
pgbench numbers.


I ran pgbench on CVS checkout taken before the patch was applied, and I 
couldn't measure a difference.


I got 1329-1340 TPM from three runs both with and without the patch. The 
tests were run on my laptop, with scaling factor 10, using pgbench 
postgres -t 10 -v, with fsync and full_page_writes disabled to make 
it CPU bound, while observing top to confirm that CPU usage was 100% 
during the test.


I think that's enough performance testing for this patch.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [pgsql-patches] [HACKERS] [PATCHES] Last infomask bit

2007-01-10 Thread Heikki Linnakangas

Bruce Momjian wrote:

Tom Lane wrote:

Bruce Momjian [EMAIL PROTECTED] writes:

Patch applied.  Thanks.
I added a comment about the unused bits in the header file.

Has anyone bothered to measure the overhead added by having to mask to
fetch or store the natts value?  This is not a zero-cost improvement.


SHOW ALL has:

   log_temp_files  | -1 | Log the 
use of temporary files larger than th

and pg_settings has:

   log_temp_files| -1  | kB  | Reporting and Logging / What to Log

Looks OK to me.


What? I'm completely lost here. What does log_temp_files have to do with 
the bits on the tuple header?


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org


Re: [pgsql-patches] [HACKERS] [PATCHES] Last infomask bit

2007-01-10 Thread Heikki Linnakangas

Tom Lane wrote:

Bruce Momjian [EMAIL PROTECTED] writes:

Tom Lane wrote:

Has anyone bothered to measure the overhead added by having to mask to
fetch or store the natts value?  This is not a zero-cost improvement.


I haven't tested it. Agreed, it does add an AND operation to places 
where t_natts is accessed.



Tom, how should this be tested?  I assume some loop of the same query
over and over again.


I'd be satisfied by a demonstration of no meaningful difference in
pgbench numbers.

It's *probably* not a problem, but you never know if you don't check...


I doubt there's any measurable difference, but I can do a pgbench run to 
make sure.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at

   http://www.postgresql.org/about/donate


Re: [pgsql-patches] [HACKERS] [PATCHES] Last infomask bit

2007-01-10 Thread Tom Lane
Heikki Linnakangas [EMAIL PROTECTED] writes:
 What? I'm completely lost here. What does log_temp_files have to do with 
 the bits on the tuple header?

Nothing, it looks like Bruce replied to the wrong message at one point
while these two threads were active ...

regards, tom lane

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [pgsql-patches] [HACKERS] [PATCHES] Last infomask bit

2007-01-10 Thread Bruce Momjian
Heikki Linnakangas wrote:
 Bruce Momjian wrote:
  Tom Lane wrote:
  Bruce Momjian [EMAIL PROTECTED] writes:
  Patch applied.  Thanks.
  I added a comment about the unused bits in the header file.
  Has anyone bothered to measure the overhead added by having to mask to
  fetch or store the natts value?  This is not a zero-cost improvement.
  
  SHOW ALL has:
  
 log_temp_files  | -1 | Log 
  the use of temporary files larger than th
  
  and pg_settings has:
  
 log_temp_files| -1  | kB  | Reporting and Logging / What to Log
  
  Looks OK to me.
 
 What? I'm completely lost here. What does log_temp_files have to do with 
 the bits on the tuple header?

Sorry, Tom wanted two things tested and I replied to the wrong email.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [PATCHES] Last infomask bit

2007-01-09 Thread Bruce Momjian

Patch applied.  Thanks.

I added a comment about the unused bits in the header file.

---



Heikki Linnakangas wrote:
 Hi,
 
 We're running out of infomask bits in the tuple header. I bumped into 
 this as I tried to apply both the phantom command ids patch, and the HOT 
 patch simultaneously. They both require one infomask bit, so they 
 conflicted.
 
 This has been discussed before; I think the best approach is to use the 
 extra bits available in t_natts field. Here's a patch that doesn't do 
 anything interesting in itself, but it renames the t_natts field to 
 t_infomask2, with 11 bits reserved for the number of attributes and the 
 other 5 bits available for future use. All references to the old t_natts 
 field are replaced with a HeapTupleHeaderGetNatts accessor macro.
 
 I believe it would actually be even better to combine the t_natts and 
 t_infomask fields to a single 32-bit infomask field. I refrained from 
 doing that for now because it would've required shifting all the 
 existing infomask flags.
 
 Naturally, there's no point applying this before we actually need more 
 infobits, but it's good to be prepared.
 
 -- 
Heikki Linnakangas
EnterpriseDB   http://www.enterprisedb.com

[ text/x-patch is unsupported, treating like TEXT/PLAIN ]

 Index: src/backend/access/common/heaptuple.c
 ===
 RCS file: 
 /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/common/heaptuple.c,v
 retrieving revision 1.112
 diff -c -r1.112 heaptuple.c
 *** src/backend/access/common/heaptuple.c 23 Nov 2006 05:27:18 -  
 1.112
 --- src/backend/access/common/heaptuple.c 5 Jan 2007 13:11:10 -
 ***
 *** 295,301 
   bool
   heap_attisnull(HeapTuple tup, int attnum)
   {
 ! if (attnum  (int) tup-t_data-t_natts)
   return true;
   
   if (attnum  0)
 --- 295,301 
   bool
   heap_attisnull(HeapTuple tup, int attnum)
   {
 ! if (attnum  (int) HeapTupleHeaderGetNatts(tup-t_data))
   return true;
   
   if (attnum  0)
 ***
 *** 474,479 
 --- 474,480 
   {
   int j = 1;
   longoff;
 + int natts = HeapTupleHeaderGetNatts(tup);
   
   /*
* need to set cache for some atts
 ***
 *** 488,494 
   
   for (; j = attnum ||
   /* Can we compute more?  We will probably need them */
 !  (j  tup-t_natts 
 att[j]-attcacheoff == -1 
 (HeapTupleNoNulls(tuple) || !att_isnull(j, bp)) 
 (HeapTupleAllFixed(tuple) || att[j]-attlen  0)); 
 j++)
 --- 489,495 
   
   for (; j = attnum ||
   /* Can we compute more?  We will probably need them */
 !  (j  natts 
 att[j]-attcacheoff == -1 
 (HeapTupleNoNulls(tuple) || !att_isnull(j, bp)) 
 (HeapTupleAllFixed(tuple) || att[j]-attlen  0)); 
 j++)
 ***
 *** 739,745 
   HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
   HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
   
 ! td-t_natts = numberOfAttributes;
   td-t_hoff = hoff;
   
   if (tupleDescriptor-tdhasoid)  /* else leave infomask = 0 */
 --- 740,746 
   HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
   HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
   
 ! HeapTupleHeaderSetNatts(td, numberOfAttributes);
   td-t_hoff = hoff;
   
   if (tupleDescriptor-tdhasoid)  /* else leave infomask = 0 */
 ***
 *** 846,852 
   HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
   HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
   
 ! td-t_natts = numberOfAttributes;
   td-t_hoff = hoff;
   
   if (tupleDescriptor-tdhasoid)  /* else leave infomask = 0 */
 --- 847,853 
   HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
   HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
   
 ! HeapTupleHeaderSetNatts(td, numberOfAttributes);
   td-t_hoff = hoff;
   
   if (tupleDescriptor-tdhasoid)  /* else leave infomask = 0 */
 ***
 *** 1035,1041 
   bits8  *bp = tup-t_bits;   /* ptr to null bitmap in tuple 
 */
   boolslow = false;   /* can we use/set attcacheoff? */
   
 ! natts = tup-t_natts;
   
   /*
* In inheritance situations, it is possible that the given tuple 
 actually
 --- 1036,1042 
   bits8  *bp = tup-t_bits;   /* ptr to null bitmap in tuple 
 */
   boolslow = false;   /* can we use/set attcacheoff? */
   
 ! natts = HeapTupleHeaderGetNatts(tup);
   

Re: [PATCHES] Last infomask bit

2007-01-09 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes:
 Patch applied.  Thanks.
 I added a comment about the unused bits in the header file.

Has anyone bothered to measure the overhead added by having to mask to
fetch or store the natts value?  This is not a zero-cost improvement.

regards, tom lane

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster


Re: [HACKERS] [PATCHES] Last infomask bit

2007-01-09 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian [EMAIL PROTECTED] writes:
  Patch applied.  Thanks.
  I added a comment about the unused bits in the header file.
 
 Has anyone bothered to measure the overhead added by having to mask to
 fetch or store the natts value?  This is not a zero-cost improvement.

I assumed Heikki had tested it, but now see no mention of a test in the
posting:

http://archives.postgresql.org/pgsql-patches/2007-01/msg00052.php

Tom, how should this be tested?  I assume some loop of the same query
over and over again.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [HACKERS] [PATCHES] Last infomask bit

2007-01-09 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian [EMAIL PROTECTED] writes:
  Patch applied.  Thanks.
  I added a comment about the unused bits in the header file.
 
 Has anyone bothered to measure the overhead added by having to mask to
 fetch or store the natts value?  This is not a zero-cost improvement.

SHOW ALL has:

   log_temp_files  | -1 | Log the 
use of temporary files larger than th

and pg_settings has:

   log_temp_files| -1  | kB  | Reporting and Logging / What to Log

Looks OK to me.

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org


Re: [HACKERS] [PATCHES] Last infomask bit

2007-01-09 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes:

 SHOW ALL has:

log_temp_files  | -1 | Log the 
 use of temporary files larger than th

Yeah, but if you do SET log_temp_files = -1, does it still say that?
I'm worried that will change it to -1024.

regards, tom lane

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


Re: [HACKERS] [PATCHES] Last infomask bit

2007-01-09 Thread Tom Lane
Bruce Momjian [EMAIL PROTECTED] writes:
 Tom Lane wrote:
 Has anyone bothered to measure the overhead added by having to mask to
 fetch or store the natts value?  This is not a zero-cost improvement.

 Tom, how should this be tested?  I assume some loop of the same query
 over and over again.

I'd be satisfied by a demonstration of no meaningful difference in
pgbench numbers.

It's *probably* not a problem, but you never know if you don't check...

regards, tom lane

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


Re: [HACKERS] [PATCHES] Last infomask bit

2007-01-09 Thread Bruce Momjian
Tom Lane wrote:
 Bruce Momjian [EMAIL PROTECTED] writes:
 
  SHOW ALL has:
 
 log_temp_files  | -1 | Log 
  the use of temporary files larger than th
 
 Yeah, but if you do SET log_temp_files = -1, does it still say that?
 I'm worried that will change it to -1024.

You can rest easy tonight.  :-)

test= SET log_temp_files = -1;
SET
test= SHOW log_temp_files;
 log_temp_files

 -1
(1 row)

test= SET log_temp_files = 1;
SET
test= SHOW log_temp_files;
 log_temp_files

 1kB
(1 row)

-- 
  Bruce Momjian   [EMAIL PROTECTED]
  EnterpriseDBhttp://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
   subscribe-nomail command to [EMAIL PROTECTED] so that your
   message can get through to the mailing list cleanly


[PATCHES] Last infomask bit

2007-01-05 Thread Heikki Linnakangas

Hi,

We're running out of infomask bits in the tuple header. I bumped into 
this as I tried to apply both the phantom command ids patch, and the HOT 
patch simultaneously. They both require one infomask bit, so they 
conflicted.


This has been discussed before; I think the best approach is to use the 
extra bits available in t_natts field. Here's a patch that doesn't do 
anything interesting in itself, but it renames the t_natts field to 
t_infomask2, with 11 bits reserved for the number of attributes and the 
other 5 bits available for future use. All references to the old t_natts 
field are replaced with a HeapTupleHeaderGetNatts accessor macro.


I believe it would actually be even better to combine the t_natts and 
t_infomask fields to a single 32-bit infomask field. I refrained from 
doing that for now because it would've required shifting all the 
existing infomask flags.


Naturally, there's no point applying this before we actually need more 
infobits, but it's good to be prepared.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
Index: src/backend/access/common/heaptuple.c
===
RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/common/heaptuple.c,v
retrieving revision 1.112
diff -c -r1.112 heaptuple.c
*** src/backend/access/common/heaptuple.c	23 Nov 2006 05:27:18 -	1.112
--- src/backend/access/common/heaptuple.c	5 Jan 2007 13:11:10 -
***
*** 295,301 
  bool
  heap_attisnull(HeapTuple tup, int attnum)
  {
! 	if (attnum  (int) tup-t_data-t_natts)
  		return true;
  
  	if (attnum  0)
--- 295,301 
  bool
  heap_attisnull(HeapTuple tup, int attnum)
  {
! 	if (attnum  (int) HeapTupleHeaderGetNatts(tup-t_data))
  		return true;
  
  	if (attnum  0)
***
*** 474,479 
--- 474,480 
  	{
  		int			j = 1;
  		long		off;
+ 		int			natts = HeapTupleHeaderGetNatts(tup);
  
  		/*
  		 * need to set cache for some atts
***
*** 488,494 
  
  		for (; j = attnum ||
  		/* Can we compute more?  We will probably need them */
! 			 (j  tup-t_natts 
  			  att[j]-attcacheoff == -1 
  			  (HeapTupleNoNulls(tuple) || !att_isnull(j, bp)) 
  			  (HeapTupleAllFixed(tuple) || att[j]-attlen  0)); j++)
--- 489,495 
  
  		for (; j = attnum ||
  		/* Can we compute more?  We will probably need them */
! 			 (j  natts 
  			  att[j]-attcacheoff == -1 
  			  (HeapTupleNoNulls(tuple) || !att_isnull(j, bp)) 
  			  (HeapTupleAllFixed(tuple) || att[j]-attlen  0)); j++)
***
*** 739,745 
  	HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
  	HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
  
! 	td-t_natts = numberOfAttributes;
  	td-t_hoff = hoff;
  
  	if (tupleDescriptor-tdhasoid)		/* else leave infomask = 0 */
--- 740,746 
  	HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
  	HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
  
! 	HeapTupleHeaderSetNatts(td, numberOfAttributes);
  	td-t_hoff = hoff;
  
  	if (tupleDescriptor-tdhasoid)		/* else leave infomask = 0 */
***
*** 846,852 
  	HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
  	HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
  
! 	td-t_natts = numberOfAttributes;
  	td-t_hoff = hoff;
  
  	if (tupleDescriptor-tdhasoid)		/* else leave infomask = 0 */
--- 847,853 
  	HeapTupleHeaderSetTypeId(td, tupleDescriptor-tdtypeid);
  	HeapTupleHeaderSetTypMod(td, tupleDescriptor-tdtypmod);
  
! 	HeapTupleHeaderSetNatts(td, numberOfAttributes);
  	td-t_hoff = hoff;
  
  	if (tupleDescriptor-tdhasoid)		/* else leave infomask = 0 */
***
*** 1035,1041 
  	bits8	   *bp = tup-t_bits;		/* ptr to null bitmap in tuple */
  	bool		slow = false;	/* can we use/set attcacheoff? */
  
! 	natts = tup-t_natts;
  
  	/*
  	 * In inheritance situations, it is possible that the given tuple actually
--- 1036,1042 
  	bits8	   *bp = tup-t_bits;		/* ptr to null bitmap in tuple */
  	bool		slow = false;	/* can we use/set attcacheoff? */
  
! 	natts = HeapTupleHeaderGetNatts(tup);
  
  	/*
  	 * In inheritance situations, it is possible that the given tuple actually
***
*** 1128,1134 
  	bits8	   *bp = tup-t_bits;		/* ptr to null bitmap in tuple */
  	bool		slow = false;	/* can we use/set attcacheoff? */
  
! 	natts = tup-t_natts;
  
  	/*
  	 * In inheritance situations, it is possible that the given tuple actually
--- 1129,1135 
  	bits8	   *bp = tup-t_bits;		/* ptr to null bitmap in tuple */
  	bool		slow = false;	/* can we use/set attcacheoff? */
  
! 	natts = HeapTupleHeaderGetNatts(tup);
  
  	/*
  	 * In inheritance situations, it is possible that the given tuple actually
***
*** 1335,1341 
  	 * than the tupdesc.)
  	 */
  	tup = tuple-t_data;
! 	if (attnum  tup-t_natts)
  	{
  		*isnull = true;
  		return (Datum) 0;
--- 1336,1342 
  	 * than the tupdesc.)
  	 */
  	tup = tuple-t_data;
! 	

Re: [PATCHES] Last infomask bit

2007-01-05 Thread Tom Lane
Heikki Linnakangas [EMAIL PROTECTED] writes:
  I believe it would actually be even better to combine the t_natts and 
 t_infomask fields to a single 32-bit infomask field.

That's not happening, because the alignment is wrong ...unless maybe
we switch this field to fall before t_ctid, but that would screw up
the MinimalTuple hack.

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings