Bug in 6.4 release

So it seems that there is a problem with using indexes in ORs that are
defined over text types

the same crash happened also when using varchar(1) as the type of
state_id

BTW, it does not happen when the state_id is first field

Recreated. I am on it.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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

Hi

I have the following problem using PostgreSQL 6.4 on RedHat Linux 5.1
on x86

using the following table

thplus=> \d envelope

Table    = envelope
+-------------------------+----------------------------------+-------+
|           Field         |              Type                | Length|
+-------------------------+----------------------------------+-------+
| envelope_id             | int4 not null default nextval (  |     4 |
| order_type_id           | int4 not null                    |     4 |
| envelope_name           | varchar() not null               |    32 |
| signed_string           | text                             |   var |
| envelope_comment        | text                             |   var |
| envelope_on_hold        | int2                             |     2 |
| envelope_order_count    | int4                             |     4 |
| envelope_total          | int4                             |     4 |
| envelope_currency       | text                             |   var |
| envelope_modify_time    | datetime                         |     8 |
| state_id                | char()                           |     1 |
+-------------------------+----------------------------------+-------+

thplus=> create index envelope_fk2 on envelope(state_id)

I try to use the following query

thplus=>
explain
thplus-> select count(*) from envelope where state_id='H' or
state_id='E';
NOTICE: QUERY PLAN:

Aggregate (cost=4.10 size=0 width=0)
-> Index Scan using envelope_fk2 on envelope (cost=4.10 size=1
width=4)

EXPLAIN

when actually running it, I get the following:

thplus=> select count(*) from envelope where state_id='H' or
state_id='E';
pqReadData() -- backend closed the channel unexpectedly.
This probably means the backend terminated abnormally before or
while processing the request.
We have lost the connection to the backend, so further processing is
impossible. Terminating.

But the following query runs fine:

thplu=> select count(*) from envelope where envelope_id=1 or
envelope_id=3;
count
-----
2
(1 row)

as well as this

thplus=> select count(*) from envelope where envelope_id=1 or
state_id='E';
count
-----
12
(1 row)

and this

thplus=> select count(*) from envelope where state_id='H'
thplus-> union
thplus-> select count(*) from envelope where state_id='E';
count
-----
11
1140
(2 rows)

So it seems that there is a problem with using indexes in ORs that are
defined over text types

the same crash happened also when using varchar(1) as the type of
state_id

BTW, it does not happen when the state_id is first field

--------------
Hannu

I need help with this one. Attached is a patch that also fails, but it
looks closer than the original code. The problem appears to be that I
can't get a slot that matches the items of the Var node I am trying to
evaluate. If I used one that matches the heap tuple, that fails,
because if the index is on the second column of the tuple, the attnum is
1, while it is actually 2nd in the tuple slot.

Does anyone know the executor well enough to find me that slot that
matches the Var node? I can't figure it out.

---------------------------------------------------------------------------

*** ./backend/executor/nodeIndexscan.c.orig	Fri Nov 20 11:38:27 1998
--- ./backend/executor/nodeIndexscan.c	Fri Nov 20 13:25:46 1998
***************
*** 153,161 ****
  				for (prev_index = 0; prev_index < indexstate->iss_IndexPtr;
  					 prev_index++)
  				{
- 					scanstate->cstate.cs_ExprContext->ecxt_scantuple = slot;
  					if (ExecQual(nth(prev_index, node->indxqual),
! 								 scanstate->cstate.cs_ExprContext))
  					{
  						prev_matches = true;
  						break;
--- 153,160 ----
  				for (prev_index = 0; prev_index < indexstate->iss_IndexPtr;
  					 prev_index++)
  				{
  					if (ExecQual(nth(prev_index, node->indxqual),
! 								node->scan.scanstate->cstate.cs_ExprContext))
  					{
  						prev_matches = true;
  						break;

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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:

thplus=> select count(*) from envelope where state_id='H' or
state_id='E';
pqReadData() -- backend closed the channel unexpectedly.
This probably means the backend terminated abnormally before or
while processing the request.
We have lost the connection to the backend, so further processing is
impossible. Terminating.

I need help with this one. Attached is a patch that also fails, but it
looks closer than the original code. The problem appears to be that I
can't get a slot that matches the items of the Var node I am trying to
evaluate. If I used one that matches the heap tuple, that fails,
because if the index is on the second column of the tuple, the attnum is
1, while it is actually 2nd in the tuple slot.

Does anyone know the executor well enough to find me that slot that
matches the Var node? I can't figure it out.

Hi, Bruce!

I'll take a look...

Vadim

Thanks.

First, let me give you a reproducible example:

create table test (x int, y text);
insert into test values (1,'fred');
insert into test values (1,'barney');
insert into test select * from test;
..repeat the above several times
create index i_test on test(y);
vacuum;
select count(*) from test where y='fred' or y='barney';
..crash

This only crashes if the OR field is not the first field in the table.
The error appears to be that the indexqual has a varno of 1, while the
heap location of the column in the above example is obviously 2. My
guess is that the varno matches the index varno, and not the heap varno.

I not think previous patch is wrong. The original code is better. You
have to compare the qualification against the actual heap row, because
you could be using a different index on the second OR than the first:

This new patch uses scankeys instead of throwing the indexqual to the
executor. This is probably more efficient, but I am not sure about the
other ramifications. It still fails.

Obviously, the idea that I want to use indexqual to test a heap tuple
was never anticipated by the original coders. I wonder why more people
have not reported OR problems?

Good luck. I am kind of stumped on this.

---------------------------------------------------------------------------

Index: src/backend/executor/nodeIndexscan.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/executor/nodeIndexscan.c,v
retrieving revision 1.27
diff -c -r1.27 nodeIndexscan.c
*** nodeIndexscan.c	1998/09/01 04:28:32	1.27
--- nodeIndexscan.c	1998/11/22 05:30:40
***************
*** 39,44 ****
--- 39,45 ----

  #include "access/skey.h"
  #include "access/heapam.h"
+ #include "access/valid.h"
  #include "access/genam.h"
  #include "utils/palloc.h"
  #include "utils/mcxt.h"
***************
*** 95,101 ****
  	TupleTableSlot *slot;
  	Buffer		buffer = InvalidBuffer;
  	int			numIndices;
! 
  	/* ----------------
  	 *	extract necessary information from index scan node
  	 * ----------------
--- 96,104 ----
  	TupleTableSlot *slot;
  	Buffer		buffer = InvalidBuffer;
  	int			numIndices;
!  	int		   *numScanKeys;
! 	ScanKey    *scanKeys;
! 	
  	/* ----------------
  	 *	extract necessary information from index scan node
  	 * ----------------
***************
*** 109,114 ****
--- 112,119 ----
  	heapRelation = scanstate->css_currentRelation;
  	numIndices = indexstate->iss_NumIndices;
  	slot = scanstate->css_ScanTupleSlot;
+ 	numScanKeys = indexstate->iss_NumScanKeys;
+ 	scanKeys = indexstate->iss_ScanKeys;

  	/* ----------------
  	 *	ok, now that we have what we need, fetch an index tuple.
***************
*** 153,161 ****
  				for (prev_index = 0; prev_index < indexstate->iss_IndexPtr;
  					 prev_index++)
  				{
! 					scanstate->cstate.cs_ExprContext->ecxt_scantuple = slot;
! 					if (ExecQual(nth(prev_index, node->indxqual),
! 								 scanstate->cstate.cs_ExprContext))
  					{
  						prev_matches = true;
  						break;
--- 158,169 ----
  				for (prev_index = 0; prev_index < indexstate->iss_IndexPtr;
  					 prev_index++)
  				{
! 					bool result;
! 					
! 					HeapKeyTest(tuple, RelationGetDescr(heapRelation),
! 								numScanKeys[prev_index],
! 								scanKeys[prev_index], result);
! 					if (result)
  					{
  						prev_matches = true;
  						break;

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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:

This new patch uses scankeys instead of throwing the indexqual to the
executor. This is probably more efficient, but I am not sure about the
other ramifications. It still fails.

This wouldn't handle functional indices in OR...

So, I added indexqualorig list to the IndexScan node:

indexqual = fix_indxqual_references (indexqualorig)

- indxqualorig' Var-s references heap tuple...

Regression tests are ok.
Patch made for CURRENT tree - let me know if there will
be problems with 6.4...

It's better to gmake clean in backend dir...

Patch also fixes EXPLAIN for indices in OR: all indices
used are explained now.

Vadim

Vadim Mikheev wrote:

Bruce Momjian wrote:

This new patch uses scankeys instead of throwing the indexqual to the
executor. This is probably more efficient, but I am not sure about the
other ramifications. It still fails.

This wouldn't handle functional indices in OR...

So, I added indexqualorig list to the IndexScan node:

indexqual = fix_indxqual_references (indexqualorig)

- indxqualorig' Var-s references heap tuple...

Regression tests are ok.

Great job! Many thanks.

And you helped me to prove to my collegues that usually bugs in
OpenSource
software are fixed no later than the next weekend ;)

Patch made for CURRENT tree - let me know if there will
be problems with 6.4...

I did'nt run regress but the patches did apply cleanly on 6.4.0 and
also all my apps run fine, even the one that did have the OR problems
after upgrading 6.3.2 -> 6.4.

And of course they run faster ;)

Thank you once more !

-------------
Hannu Krosing

Marc, please apply this to the 6.4 stable tree. I think we need it to
prevent backend crashes with OR.

My cvs on stable is not working either.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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:

This new patch uses scankeys instead of throwing the indexqual to the
executor. This is probably more efficient, but I am not sure about the
other ramifications. It still fails.

This wouldn't handle functional indices in OR...

So, I added indexqualorig list to the IndexScan node:

indexqual = fix_indxqual_references (indexqualorig)

- indxqualorig' Var-s references heap tuple...

Regression tests are ok.
Patch made for CURRENT tree - let me know if there will
be problems with 6.4...

It's better to gmake clean in backend dir...

Patch also fixes EXPLAIN for indices in OR: all indices
used are explained now.

Vadim

Thanks very much. I have applied this to the stable branch, because
without it, we get backend crashes.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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:

Thanks very much. I have applied this to the stable branch, because
without it, we get backend crashes.

Thanks, Bruce! I have only CURRENT tree...

Vadim

Bruce Momjian wrote:

Thanks very much. I have applied this to the stable branch, because
without it, we get backend crashes.

Thanks, Bruce! I have only CURRENT tree...

Glad to hear you are OK about appying it to stable tree.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  maillist@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