Subject: Re: CURRENT OF causes an error when IndexOnlyScan is used
To: pgsql-hackers@lists.postgresql.org
References: <20180201013349.937dfc5f.nagata@sraoss.co.jp>
 <20180201014444.f78358df.nagata@sraoss.co.jp>
 <22958.1517451171@sss.pgh.pa.us>
 <97dc81d3-0ea9-3032-49d9-7983e35fb010@postgrespro.ru>
 <20180220095252.GA18315@e733.localdomain>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <a6ddf99c-6579-e65a-6abd-c0b6259c15c2@postgrespro.ru>
Date: Wed, 21 Feb 2018 17:33:55 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <20180220095252.GA18315@e733.localdomain>
Content-Type: multipart/mixed;
 boundary="------------7CFBE5D1C10C3B21C4C60707"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------7CFBE5D1C10C3B21C4C60707
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

20.02.2018 12:52, Aleksander Alekseev:
> Hi Anastasia,
>
>> I'd like to propose the patch that fixes the issue.
>> We already have a way to return heaptuple from IndexOnlyScan,
>> but it was not applied to b-tree for some reason.
>>
>> Attached patch solves the reported bug.
>> Moreover, it will come in handy for "index with included attributes" feature
>> [1],
>> where we can store long (and even TOASTed) attributes in indextuple.
>>
>> [1] https://commitfest.postgresql.org/17/1350/
> I believe the patch should include a test that tries to reproduce an
> issue it tries to fix.
>
> Also maybe this code that repeats 3 times can be moved to a separate
> procedure?

Good point. Updated version with test is attached.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------7CFBE5D1C10C3B21C4C60707
Content-Type: text/x-patch;
 name="return_heaptuple_in_btree_indexonlyscan_v2.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="return_heaptuple_in_btree_indexonlyscan_v2.patch"

commit 391107028d4597e9721a3b274904e49ee69de0cf
Author: Anastasia <a.lubennikova@postgrespro.ru>
Date:   Wed Feb 21 17:25:11 2018 +0300

    return_heaptuple_in_btree_indexonlyscan_v2.patch

diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 8158508..db8a55c 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -374,6 +374,8 @@ btbeginscan(Relation rel, int nkeys, int norderbys)
 	so->currTuples = so->markTuples = NULL;
 
 	scan->xs_itupdesc = RelationGetDescr(rel);
+	scan->xs_hitupdesc = NULL;
+	scan->xs_hitup = NULL;
 
 	scan->opaque = so;
 
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index 51dca64..278f43d 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -25,6 +25,9 @@
 #include "utils/tqual.h"
 
 
+static HeapTuple _bt_fetch_tuple(IndexScanDesc scandesc,
+								 ItemPointerData heapTid);
+static void	_bt_fill_hitupdesc(IndexScanDesc scan);
 static bool _bt_readpage(IndexScanDesc scan, ScanDirection dir,
 			 OffsetNumber offnum);
 static void _bt_saveitem(BTScanOpaque so, int itemIndex,
@@ -38,7 +41,52 @@ static bool _bt_endpoint(IndexScanDesc scan, ScanDirection dir);
 static void _bt_drop_lock_and_maybe_pin(IndexScanDesc scan, BTScanPos sp);
 static inline void _bt_initialize_more_data(BTScanOpaque so, ScanDirection dir);
 
+/*
+ * Fetch all keys in tuple.
+ * Returns a new HeapTuple containing the originally-indexed data.
+ */
+static HeapTuple
+_bt_fetch_tuple(IndexScanDesc scandesc, ItemPointerData heapTid)
+{
+	Relation index = scandesc->indexRelation;
+	Datum		fetchatt[INDEX_MAX_KEYS];
+	bool		isnull[INDEX_MAX_KEYS];
+	int			i;
+	HeapTuple htuple;
+
+	for (i = 0; i < index->rd_att->natts; i++)
+	{
+		fetchatt[i] = index_getattr(scandesc->xs_itup, i + 1,
+									scandesc->xs_itupdesc, &isnull[i]);
+	}
+
+	htuple = heap_form_tuple(scandesc->xs_hitupdesc, fetchatt, isnull);
+	htuple->t_tableOid = scandesc->heapRelation->rd_id;
+	htuple->t_self = heapTid;
 
+
+	return htuple;
+}
+
+static void
+_bt_fill_hitupdesc(IndexScanDesc scan)
+{
+	int natts = RelationGetNumberOfAttributes(scan->indexRelation);
+	int attno;
+	/*
+	* The storage type of the index can be different from the original
+	* datatype being indexed, so we cannot just grab the index's tuple
+	* descriptor. Instead, construct a descriptor with the original data
+	* types.
+	*/
+	scan->xs_hitupdesc = CreateTemplateTupleDesc(natts, false);
+	for (attno = 1; attno <= natts; attno++)
+	{
+		TupleDescInitEntry(scan->xs_hitupdesc, attno, NULL,
+						scan->indexRelation->rd_opcintype[attno - 1],
+						-1, 0);
+	}
+}
 /*
  *	_bt_drop_lock_and_maybe_pin()
  *
@@ -1105,9 +1153,19 @@ readcomplete:
 	/* OK, itemIndex says what to return */
 	currItem = &so->currPos.items[so->currPos.itemIndex];
 	scan->xs_ctup.t_self = currItem->heapTid;
+
 	if (scan->xs_want_itup)
+	{
 		scan->xs_itup = (IndexTuple) (so->currTuples + currItem->tupleOffset);
 
+		if (!scan->xs_hitupdesc)
+			_bt_fill_hitupdesc(scan);
+
+		if (scan->xs_hitup)
+			pfree(scan->xs_hitup);
+		scan->xs_hitup = _bt_fetch_tuple(scan, currItem->heapTid);
+	}
+
 	return true;
 }
 
@@ -1155,9 +1213,19 @@ _bt_next(IndexScanDesc scan, ScanDirection dir)
 	/* OK, itemIndex says what to return */
 	currItem = &so->currPos.items[so->currPos.itemIndex];
 	scan->xs_ctup.t_self = currItem->heapTid;
+
 	if (scan->xs_want_itup)
+	{
 		scan->xs_itup = (IndexTuple) (so->currTuples + currItem->tupleOffset);
 
+		if (!scan->xs_hitupdesc)
+			_bt_fill_hitupdesc(scan);
+
+		if (scan->xs_hitup)
+			pfree(scan->xs_hitup);
+		scan->xs_hitup = _bt_fetch_tuple(scan, currItem->heapTid);
+	}
+
 	return true;
 }
 
@@ -1932,9 +2000,19 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
 	/* OK, itemIndex says what to return */
 	currItem = &so->currPos.items[so->currPos.itemIndex];
 	scan->xs_ctup.t_self = currItem->heapTid;
+
 	if (scan->xs_want_itup)
+	{
 		scan->xs_itup = (IndexTuple) (so->currTuples + currItem->tupleOffset);
 
+		if (!scan->xs_hitupdesc)
+			_bt_fill_hitupdesc(scan);
+
+		if (scan->xs_hitup)
+			pfree(scan->xs_hitup);
+		scan->xs_hitup = _bt_fetch_tuple(scan, currItem->heapTid);
+	}
+
 	return true;
 }
 
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index 1b8f7b6..d01db92 100644
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -1352,3 +1352,25 @@ fetch backward all in c2;
 (3 rows)
 
 rollback;
+create table  cursor_tbl (i int);
+insert into cursor_tbl values (1);
+VACUUM cursor_tbl;
+begin;
+set enable_seqscan to off;
+declare c cursor for select * from cursor_tbl where i = 1;
+fetch from c;
+ i 
+---
+ 1
+(1 row)
+
+update cursor_tbl set i=i+1 where current of c;
+select * from cursor_tbl;
+ i 
+---
+ 2
+(1 row)
+
+set enable_seqscan to on;
+rollback;
+drop table  cursor_tbl;
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index a1c812e..78fee8c 100644
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -500,3 +500,16 @@ declare c2 scroll cursor for select generate_series(1,3) as g;
 fetch all in c2;
 fetch backward all in c2;
 rollback;
+
+create table  cursor_tbl (i int);
+insert into cursor_tbl values (1);
+VACUUM cursor_tbl;
+begin;
+set enable_seqscan to off;
+declare c cursor for select * from cursor_tbl where i = 1;
+fetch from c;
+update cursor_tbl set i=i+1 where current of c;
+select * from cursor_tbl;
+set enable_seqscan to on;
+rollback;
+drop table  cursor_tbl;

--------------7CFBE5D1C10C3B21C4C60707--