Last infomask bit

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

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> 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

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 -0000 1.112
> --- src/backend/access/common/heaptuple.c 5 Jan 2007 13:11:10 -0000
> ***************
> *** 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;
> ! if (attnum > HeapTupleHeaderGetNatts(tup))
> {
> *isnull = true;
> return (Datum) 0;
> ***************
> *** 1401,1407 ****
> /*
> * load up any slots available from physical tuple
> */
> ! attnum = tuple->t_data->t_natts;
> attnum = Min(attnum, tdesc_natts);
>
> slot_deform_tuple(slot, attnum);
> --- 1402,1408 ----
> /*
> * load up any slots available from physical tuple
> */
> ! attnum = HeapTupleHeaderGetNatts(tuple->t_data);
> attnum = Min(attnum, tdesc_natts);
>
> slot_deform_tuple(slot, attnum);
> ***************
> *** 1448,1454 ****
> /*
> * load up any slots available from physical tuple
> */
> ! attno = tuple->t_data->t_natts;
> attno = Min(attno, attnum);
>
> slot_deform_tuple(slot, attno);
> --- 1449,1455 ----
> /*
> * load up any slots available from physical tuple
> */
> ! attno = HeapTupleHeaderGetNatts(tuple->t_data);
> attno = Min(attno, attnum);
>
> slot_deform_tuple(slot, attno);
> ***************
> *** 1601,1607 ****
> * And fill in the information.
> */
> tuple->t_len = len;
> ! tuple->t_natts = numberOfAttributes;
> tuple->t_hoff = hoff + MINIMAL_TUPLE_OFFSET;
>
> if (tupleDescriptor->tdhasoid) /* else leave infomask = 0 */
> --- 1602,1608 ----
> * And fill in the information.
> */
> tuple->t_len = len;
> ! HeapTupleHeaderSetNatts(tuple, numberOfAttributes);
> tuple->t_hoff = hoff + MINIMAL_TUPLE_OFFSET;
>
> if (tupleDescriptor->tdhasoid) /* else leave infomask = 0 */
> ***************
> *** 1663,1669 ****
> result->t_tableOid = InvalidOid;
> result->t_data = (HeapTupleHeader) ((char *) result + HEAPTUPLESIZE);
> memcpy((char *) result->t_data + MINIMAL_TUPLE_OFFSET, mtup, mtup->t_len);
> ! memset(result->t_data, 0, offsetof(HeapTupleHeaderData, t_natts));
> return result;
> }
>
> --- 1664,1670 ----
> result->t_tableOid = InvalidOid;
> result->t_data = (HeapTupleHeader) ((char *) result + HEAPTUPLESIZE);
> memcpy((char *) result->t_data + MINIMAL_TUPLE_OFFSET, mtup, mtup->t_len);
> ! memset(result->t_data, 0, offsetof(HeapTupleHeaderData, t_infomask2));
> return result;
> }
>
> ***************
> *** 1729,1735 ****
>
> /* we don't bother to fill the Datum fields */
>
> ! td->t_natts = natts;
> td->t_hoff = hoff;
>
> if (withoid) /* else leave infomask = 0 */
> --- 1730,1736 ----
>
> /* we don't bother to fill the Datum fields */
>
> ! HeapTupleHeaderSetNatts(td, natts);
> td->t_hoff = hoff;
>
> if (withoid) /* else leave infomask = 0 */
> Index: src/backend/access/heap/heapam.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/access/heap/heapam.c,v
> retrieving revision 1.222
> diff -c -r1.222 heapam.c
> *** src/backend/access/heap/heapam.c 17 Nov 2006 18:00:14 -0000 1.222
> --- src/backend/access/heap/heapam.c 5 Jan 2007 13:13:30 -0000
> ***************
> *** 1455,1461 ****
> rdata[0].buffer = InvalidBuffer;
> rdata[0].next = &(rdata[1]);
>
> ! xlhdr.t_natts = heaptup->t_data->t_natts;
> xlhdr.t_infomask = heaptup->t_data->t_infomask;
> xlhdr.t_hoff = heaptup->t_data->t_hoff;
>
> --- 1455,1461 ----
> rdata[0].buffer = InvalidBuffer;
> rdata[0].next = &(rdata[1]);
>
> ! xlhdr.t_infomask2 = heaptup->t_data->t_infomask2;
> xlhdr.t_infomask = heaptup->t_data->t_infomask;
> xlhdr.t_hoff = heaptup->t_data->t_hoff;
>
> ***************
> *** 3204,3210 ****
> rdata[1].buffer_std = true;
> rdata[1].next = &(rdata[2]);
>
> ! xlhdr.hdr.t_natts = newtup->t_data->t_natts;
> xlhdr.hdr.t_infomask = newtup->t_data->t_infomask;
> xlhdr.hdr.t_hoff = newtup->t_data->t_hoff;
> if (move) /* remember xmax & xmin */
> --- 3204,3210 ----
> rdata[1].buffer_std = true;
> rdata[1].next = &(rdata[2]);
>
> ! xlhdr.hdr.t_infomask2 = newtup->t_data->t_infomask2;
> xlhdr.hdr.t_infomask = newtup->t_data->t_infomask;
> xlhdr.hdr.t_hoff = newtup->t_data->t_hoff;
> if (move) /* remember xmax & xmin */
> ***************
> *** 3503,3509 ****
> (char *) xlrec + SizeOfHeapInsert + SizeOfHeapHeader,
> newlen);
> newlen += offsetof(HeapTupleHeaderData, t_bits);
> ! htup->t_natts = xlhdr.t_natts;
> htup->t_infomask = xlhdr.t_infomask;
> htup->t_hoff = xlhdr.t_hoff;
> HeapTupleHeaderSetXmin(htup, record->xl_xid);
> --- 3503,3509 ----
> (char *) xlrec + SizeOfHeapInsert + SizeOfHeapHeader,
> newlen);
> newlen += offsetof(HeapTupleHeaderData, t_bits);
> ! htup->t_infomask2 = xlhdr.t_infomask2;
> htup->t_infomask = xlhdr.t_infomask;
> htup->t_hoff = xlhdr.t_hoff;
> HeapTupleHeaderSetXmin(htup, record->xl_xid);
> ***************
> *** 3666,3672 ****
> (char *) xlrec + hsize,
> newlen);
> newlen += offsetof(HeapTupleHeaderData, t_bits);
> ! htup->t_natts = xlhdr.t_natts;
> htup->t_infomask = xlhdr.t_infomask;
> htup->t_hoff = xlhdr.t_hoff;
>
> --- 3666,3672 ----
> (char *) xlrec + hsize,
> newlen);
> newlen += offsetof(HeapTupleHeaderData, t_bits);
> ! htup->t_infomask2 = xlhdr.t_infomask2;
> htup->t_infomask = xlhdr.t_infomask;
> htup->t_hoff = xlhdr.t_hoff;
>
> Index: src/backend/executor/spi.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/backend/executor/spi.c,v
> retrieving revision 1.165.2.2
> diff -c -r1.165.2.2 spi.c
> *** src/backend/executor/spi.c 26 Dec 2006 16:56:22 -0000 1.165.2.2
> --- src/backend/executor/spi.c 5 Jan 2007 13:16:42 -0000
> ***************
> *** 651,657 ****
>
> SPI_result = 0;
>
> ! if (fnumber > tuple->t_data->t_natts || fnumber == 0 ||
> fnumber <= FirstLowInvalidHeapAttributeNumber)
> {
> SPI_result = SPI_ERROR_NOATTRIBUTE;
> --- 651,657 ----
>
> SPI_result = 0;
>
> ! if (fnumber > HeapTupleHeaderGetNatts(tuple->t_data) || fnumber == 0 ||
> fnumber <= FirstLowInvalidHeapAttributeNumber)
> {
> SPI_result = SPI_ERROR_NOATTRIBUTE;
> ***************
> *** 692,698 ****
> {
> SPI_result = 0;
>
> ! if (fnumber > tuple->t_data->t_natts || fnumber == 0 ||
> fnumber <= FirstLowInvalidHeapAttributeNumber)
> {
> SPI_result = SPI_ERROR_NOATTRIBUTE;
> --- 692,698 ----
> {
> SPI_result = 0;
>
> ! if (fnumber > HeapTupleHeaderGetNatts(tuple->t_data) || fnumber == 0 ||
> fnumber <= FirstLowInvalidHeapAttributeNumber)
> {
> SPI_result = SPI_ERROR_NOATTRIBUTE;
> Index: src/include/access/heapam.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/heapam.h,v
> retrieving revision 1.117
> diff -c -r1.117 heapam.h
> *** src/include/access/heapam.h 5 Nov 2006 22:42:10 -0000 1.117
> --- src/include/access/heapam.h 5 Jan 2007 13:15:10 -0000
> ***************
> *** 98,104 ****
> ( \
> ((attnum) > 0) ? \
> ( \
> ! ((attnum) > (int) (tup)->t_data->t_natts) ? \
> ( \
> (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \
> (Datum)NULL \
> --- 98,104 ----
> ( \
> ((attnum) > 0) ? \
> ( \
> ! ((attnum) > (int) HeapTupleHeaderGetNatts((tup)->t_data)) ? \
> ( \
> (((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \
> (Datum)NULL \
> Index: src/include/access/htup.h
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/include/access/htup.h,v
> retrieving revision 1.87
> diff -c -r1.87 htup.h
> *** src/include/access/htup.h 5 Nov 2006 22:42:10 -0000 1.87
> --- src/include/access/htup.h 5 Jan 2007 13:08:41 -0000
> ***************
> *** 139,145 ****
>
> /* Fields below here must match MinimalTupleData! */
>
> ! int16 t_natts; /* number of attributes */
>
> uint16 t_infomask; /* various flag bits, see below */
>
> --- 139,145 ----
>
> /* Fields below here must match MinimalTupleData! */
>
> ! uint16 t_infomask2; /* number of attributes + various flags */
>
> uint16 t_infomask; /* various flag bits, see below */
>
> ***************
> *** 182,187 ****
> --- 182,195 ----
>
> #define HEAP_XACT_MASK 0xFFC0 /* visibility-related bits */
>
> + /* information stored in t_infomask2, and accessor macros */
> + #define HEAP_NATTS_MASK 0X7FF /* 11 bits for number of attributes */
> +
> + #define HeapTupleHeaderGetNatts(tup) ((tup)->t_infomask2 & HEAP_NATTS_MASK)
> + #define HeapTupleHeaderSetNatts(tup, natts) \
> + ( \
> + (tup)->t_infomask2 = ((tup)->t_infomask2 & ~HEAP_NATTS_MASK) | (natts) \
> + )
>
> /*
> * HeapTupleHeader accessor macros
> ***************
> *** 367,374 ****
> * and thereby prevent accidental use of the nonexistent fields.
> *
> * MinimalTupleData contains a length word, some padding, and fields matching
> ! * HeapTupleHeaderData beginning with t_natts. The padding is chosen so that
> ! * offsetof(t_natts) is the same modulo MAXIMUM_ALIGNOF in both structs.
> * This makes data alignment rules equivalent in both cases.
> *
> * When a minimal tuple is accessed via a HeapTupleData pointer, t_data is
> --- 375,382 ----
> * and thereby prevent accidental use of the nonexistent fields.
> *
> * MinimalTupleData contains a length word, some padding, and fields matching
> ! * HeapTupleHeaderData beginning with t_infomask2. The padding is chosen so that
> ! * offsetof(t_infomask2) is the same modulo MAXIMUM_ALIGNOF in both structs.
> * This makes data alignment rules equivalent in both cases.
> *
> * When a minimal tuple is accessed via a HeapTupleData pointer, t_data is
> ***************
> *** 380,388 ****
> * the MINIMAL_TUPLE_OFFSET distance. t_len does not include that, however.
> */
> #define MINIMAL_TUPLE_OFFSET \
> ! ((offsetof(HeapTupleHeaderData, t_natts) - sizeof(uint32)) / MAXIMUM_ALIGNOF * MAXIMUM_ALIGNOF)
> #define MINIMAL_TUPLE_PADDING \
> ! ((offsetof(HeapTupleHeaderData, t_natts) - sizeof(uint32)) % MAXIMUM_ALIGNOF)
>
> typedef struct MinimalTupleData
> {
> --- 388,396 ----
> * the MINIMAL_TUPLE_OFFSET distance. t_len does not include that, however.
> */
> #define MINIMAL_TUPLE_OFFSET \
> ! ((offsetof(HeapTupleHeaderData, t_infomask2) - sizeof(uint32)) / MAXIMUM_ALIGNOF * MAXIMUM_ALIGNOF)
> #define MINIMAL_TUPLE_PADDING \
> ! ((offsetof(HeapTupleHeaderData, t_infomask2) - sizeof(uint32)) % MAXIMUM_ALIGNOF)
>
> typedef struct MinimalTupleData
> {
> ***************
> *** 392,398 ****
>
> /* Fields below here must match HeapTupleHeaderData! */
>
> ! int16 t_natts; /* number of attributes */
>
> uint16 t_infomask; /* various flag bits, see below */
>
> --- 400,406 ----
>
> /* Fields below here must match HeapTupleHeaderData! */
>
> ! uint16 t_infomask2; /* number of attributes + various flags */
>
> uint16 t_infomask; /* various flag bits, see below */
>
> ***************
> *** 552,558 ****
> */
> typedef struct xl_heap_header
> {
> ! int16 t_natts;
> uint16 t_infomask;
> uint8 t_hoff;
> } xl_heap_header;
> --- 560,566 ----
> */
> typedef struct xl_heap_header
> {
> ! uint16 t_infomask2;
> uint16 t_infomask;
> uint8 t_hoff;
> } xl_heap_header;
> Index: src/pl/plpgsql/src/pl_exec.c
> ===================================================================
> RCS file: /home/hlinnaka/pgcvsrepository/pgsql/src/pl/plpgsql/src/pl_exec.c,v
> retrieving revision 1.180
> diff -c -r1.180 pl_exec.c
> *** src/pl/plpgsql/src/pl_exec.c 4 Oct 2006 00:30:13 -0000 1.180
> --- src/pl/plpgsql/src/pl_exec.c 5 Jan 2007 13:19:14 -0000
> ***************
> *** 4092,4098 ****
> * Row is a bit more complicated in that we assign the individual
> * attributes of the tuple to the variables the row points to.
> *
> ! * NOTE: this code used to demand row->nfields == tup->t_data->t_natts,
> * but that's wrong. The tuple might have more fields than we expected if
> * it's from an inheritance-child table of the current table, or it might
> * have fewer if the table has had columns added by ALTER TABLE. Ignore
> --- 4092,4098 ----
> * Row is a bit more complicated in that we assign the individual
> * attributes of the tuple to the variables the row points to.
> *
> ! * NOTE: this code used to demand row->nfields == HeapTupleHeaderGetNatts(tup->t_data,
> * but that's wrong. The tuple might have more fields than we expected if
> * it's from an inheritance-child table of the current table, or it might
> * have fewer if the table has had columns added by ALTER TABLE. Ignore
> ***************
> *** 4110,4116 ****
> int anum;
>
> if (HeapTupleIsValid(tup))
> ! t_natts = tup->t_data->t_natts;
> else
> t_natts = 0;
>
> --- 4110,4116 ----
> int anum;
>
> if (HeapTupleIsValid(tup))
> ! t_natts = HeapTupleHeaderGetNatts(tup->t_data);
> else
> t_natts = 0;
>

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)us> 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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> 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 bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> 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 bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian <bruce(at)momjian(dot)us> 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

Bruce Momjian <bruce(at)momjian(dot)us> 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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> 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 bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Bruce Momjian wrote:
> Tom Lane wrote:
>> Bruce Momjian <bruce(at)momjian(dot)us> 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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> 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

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> 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

Heikki Linnakangas wrote:
> Bruce Momjian wrote:
> > Tom Lane wrote:
> >> Bruce Momjian <bruce(at)momjian(dot)us> 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 bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

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

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> 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 100000 -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