Fixes to index pages

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

Still don't like the name "IndexTupleHasVars" ... sounds to me like that
means it has variables in it, which is not very sensical. Maybe
"IndexTupleHasVarlenas"?

Also, if you don't put some dashes around the comment in itup.h line
27ff, pgindent will munge it for you, just like it did for the last guy.
(Have I mentioned that I really hate pgindent's handling of comment
blocks?)

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

Still don't like the name "IndexTupleHasVars" ... sounds to me like that
means it has variables in it, which is not very sensical. Maybe
"IndexTupleHasVarlenas"?

Done. Patch attached.

Also, if you don't put some dashes around the comment in itup.h line
27ff, pgindent will munge it for you, just like it did for the last guy.
(Have I mentioned that I really hate pgindent's handling of comment
blocks?)

I see. Done. That comment wrapping is a _feature_ of BSD indent. I
can easily disable it if people don't want it. I have seen it clean up
some pretty ugly comments, but I have seen it mess a few too. I think
it does better good than harm, but others may disagree.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

You have added the following TODO recently.
* Add deleted bit to index tuples to reduce heap access

Where would you have the deleted bit in IndexTupleData ?

Regards,
Hiroshi Inoue

Bruce Momjian wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

You have added the following TODO recently.
* Add deleted bit to index tuples to reduce heap access

Where would you have the deleted bit in IndexTupleData ?

Wow, seems like everyone liked the deleted bit idea. :-)

I would put it in bit 13. I would adjust the bit masks in the itup.h
file. I assume you are asking why I don't do it in the patch, and the
reason is because I have no code to update the bit field. I think the
bit should be reserved at the time the code is added.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian wrote:

Bruce Momjian wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

You have added the following TODO recently.
* Add deleted bit to index tuples to reduce heap access

Where would you have the deleted bit in IndexTupleData ?

Wow, seems like everyone liked the deleted bit idea. :-)

I would put it in bit 13. I would adjust the bit masks in the itup.h
file. I assume you are asking why I don't do it in the patch,

I don't think it's a good idea to fill bit 13 by force.
There's only 1 bit unused. IMHO there must be a discussion
about how to use the bit.

Regards,
Hiroshi Inoue

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom, here are the changes I was thinking about to clean up a few areas
in index pages tables. I will hold the patch until 7.2.

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

You have added the following TODO recently.
* Add deleted bit to index tuples to reduce heap access

Where would you have the deleted bit in IndexTupleData ?

Wow, seems like everyone liked the deleted bit idea. :-)

I would put it in bit 13. I would adjust the bit masks in the itup.h
file. I assume you are asking why I don't do it in the patch,

I don't think it's a good idea to fill bit 13 by force.
There's only 1 bit unused. IMHO there must be a discussion
about how to use the bit.

I am not doing anything to 7.1, just 7.2. My patch is just an attempt
to make the source accurate. If you prefer, I will document the bit as
"unused" and let someone else change it later.

If I document the bit as "unused" I can apply the patch to 7.1 because
then the patch has no affect except to clean up the source labels.
Comments?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

I don't think it's a good idea to fill bit 13 by force.
There's only 1 bit unused. IMHO there must be a discussion
about how to use the bit.

OK, attached is a patch that just documents the bit as unused and
changes some poorly chosen macro names. Do people want this committed
to the current tree? Seems there is some interest in updating this area
of the code.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I don't think it's a good idea to fill bit 13 by force.
There's only 1 bit unused. IMHO there must be a discussion
about how to use the bit.

I am not doing anything to 7.1, just 7.2. My patch is just an attempt
to make the source accurate.

Hmm I've already been confused by your attempt.
For example, oops where's the discussion about changing
index tuple length limit ? etc ...
And I understand now that the original question was thrown
to Tom. I'll leave the choise to Tom.

Regards,
Hiroshi Inoue

Bruce Momjian wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I don't think it's a good idea to fill bit 13 by force.
There's only 1 bit unused. IMHO there must be a discussion
about how to use the bit.

I am not doing anything to 7.1, just 7.2. My patch is just an attempt
to make the source accurate.

Hmm I've already been confused by your attempt.
For example, oops where's the discussion about changing
index tuple length limit ? etc ...
And I understand now that the original question was thrown
to Tom. I'll leave the choise to Tom.

The patch never intended to increase the index tuple length. It was
only to better document how IndexTupleData is used. Both Tom and I
agreed that the use of bits/contants/macros in itup.h was not idea, and
needed a little cleaning. That's all the patch does.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

I object. Strongly. You are making a significant change without
discussion --- in fact, contrary to what discussion there has been.
This is not a "trivial cleanup".

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The patch never intended to increase the index tuple length. It was
only to better document how IndexTupleData is used. Both Tom and I
agreed that the use of bits/contants/macros in itup.h was not idea, and
needed a little cleaning. That's all the patch does.

The original version of the patch commandeered an extra bit for tuple
length. If you back off INDEX_SIZE_MASK to 1FFF, and document bit
13 as unused/reserved, then it's just a cleanup.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

What happened to our discussion about keeping t_info bit 13 unused??

I wasn't going to reserve it in the patch. I figured I would make all
the items/flags match, and if someone wants to reserve it, it is easy to
do in one place. I imagine 7.2 is going to be dump/reload anyway so the
decision can be made during development cycle. I basically didn't want
to leave a bit gap and leave it unnamed because it could cause
confusion.

I object. Strongly. You are making a significant change without
discussion --- in fact, contrary to what discussion there has been.
This is not a "trivial cleanup".

OK, attached is the patch. I guess I don't understand how what I am
doing affects anything.

I realize you were commenting about the earlier patch that gave the 13th
bit to the length. I now see the issue you were talking about was this
test:

/*
* Here we make sure that the size will fit in the field reserved for
* it in t_info.
*/
if ((size & INDEX_SIZE_MASK) != size)
elog(ERROR, "index_formtuple: data takes %lu bytes, max is %d",
(unsigned long) size, INDEX_SIZE_MASK);

I originally didn't realize that expanding the _storage_ space for the
index tuples actually allow storage of longer tuples. I see that now,
and this is why I just mark the patch as UNUSED. I will let others
handle it.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The patch never intended to increase the index tuple length. It was
only to better document how IndexTupleData is used. Both Tom and I
agreed that the use of bits/contants/macros in itup.h was not idea, and
needed a little cleaning. That's all the patch does.

The original version of the patch commandeered an extra bit for tuple
length. If you back off INDEX_SIZE_MASK to 1FFF, and document bit
13 as unused/reserved, then it's just a cleanup.

OK, we are both catching up now on the email. Should I put it in
current? Seems like cosmetic cleanup. Of course, even if you say yes,
I have to wait 24 hours.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

The original version of the patch commandeered an extra bit for tuple
length. If you back off INDEX_SIZE_MASK to 1FFF, and document bit
13 as unused/reserved, then it's just a cleanup.

OK, we are both catching up now on the email. Should I put it in
current? Seems like cosmetic cleanup. Of course, even if you say yes,
I have to wait 24 hours.

In the revised form I have no problem with it.

regards, tom lane