Date: Thu, 1 Feb 2018 01:44:44 +0900
From: Yugo Nagata <nagata@sraoss.co.jp>
To: Yugo Nagata <nagata@sraoss.co.jp>
Cc: pgsql-hackers@postgresql.org
Subject: Re: CURRENT OF causes an error when IndexOnlyScan is used
Message-Id: <20180201014444.f78358df.nagata@sraoss.co.jp>
In-Reply-To: <20180201013349.937dfc5f.nagata@sraoss.co.jp>
References: <20180201013349.937dfc5f.nagata@sraoss.co.jp>
Mime-Version: 1.0
Content-Type: multipart/mixed;
 boundary="Multipart=_Thu__1_Feb_2018_01_44_44_+0900_ZdDWt_639mx5cmQ9"
Precedence: bulk

This is a multi-part message in MIME format.

--Multipart=_Thu__1_Feb_2018_01_44_44_+0900_ZdDWt_639mx5cmQ9
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 1 Feb 2018 01:33:49 +0900
Yugo Nagata <nagata@sraoss.co.jp> wrote:

I'm sorry the patch attached in the previous mail is broken and
not raises a compile error. I attached the fixed patch.

Regards,

> Hi,
> 
> I found that updating a cursor by using CURRENT OF causes the
> following error when the query is executed by IndexOnlyScan. 
> 
>  ERROR:  cannot extract system attribute from virtual tuple
> 
> IndexOnlyScan returns a virtual tuple that doesn't have system
> column, so we can not get ctid in the same way of other plans.
> However, the error message is not convinient and users would
> not understand why the error occurs.
> 
> Attached is a patch to fix this. By this fix, execCurrentOf
> get ctid from IndexScanDesc->xs_ctup.t_self when the plan is
> IndexOnlyScan, and it works sucessfully without errors.
> 
> 
> Here is the example of the error:
> 
> =======
> postgres=# create table test (i int primary key);
> CREATE TABLE
> postgres=# insert into test values(1);
> INSERT 0 1
> postgres=# set enable_seqscan to off;
> SET
> 
> postgres=# explain select * from test where i = 1;
>                                 QUERY PLAN                                 
> ---------------------------------------------------------------------------
>  Index Only Scan using test_pkey on test  (cost=0.15..8.17 rows=1 width=4)
>    Index Cond: (i = 1)
> (2 rows)
> 
> postgres=# begin;
> BEGIN
> postgres=# declare c cursor for select * from test where i = 1;
> DECLARE CURSOR
> postgres=# fetch from c;
>  i 
> ---
>  1
> (1 row)
> 
> postgres=# update test set i=i+1 where current of c;
> ERROR:  cannot extract system attribute from virtual tuple
> =======
> 
> The patch fixes the error and allows this update successfully.
> 
> Regards,
> 
> -- 
> Yugo Nagata <nagata@sraoss.co.jp>


-- 
Yugo Nagata <nagata@sraoss.co.jp>

--Multipart=_Thu__1_Feb_2018_01_44_44_+0900_ZdDWt_639mx5cmQ9
Content-Type: text/x-diff;
 name="current_of_index_only_scan_v2.patch"
Content-Disposition: attachment;
 filename="current_of_index_only_scan_v2.patch"
Content-Transfer-Encoding: 7bit

diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..aa2da16 100644
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -19,6 +19,7 @@
 #include "utils/lsyscache.h"
 #include "utils/portal.h"
 #include "utils/rel.h"
+#include "access/relscan.h"
 
 
 static char *fetch_cursor_param_value(ExprContext *econtext, int paramId);
@@ -183,21 +184,35 @@ execCurrentOf(CurrentOfExpr *cexpr,
 		if (TupIsNull(scanstate->ss_ScanTupleSlot))
 			return false;
 
-		/* Use slot_getattr to catch any possible mistakes */
-		tuple_tableoid =
-			DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
-										  TableOidAttributeNumber,
-										  &lisnull));
-		Assert(!lisnull);
-		tuple_tid = (ItemPointer)
-			DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
-										 SelfItemPointerAttributeNumber,
-										 &lisnull));
-		Assert(!lisnull);
-
-		Assert(tuple_tableoid == table_oid);
-
-		*current_tid = *tuple_tid;
+		/* In IndexOnlyScan case, the tuple stored in ss_ScanTupleSlot is a
+		 * virtual tuple that does not have ctid column, so we have to get TID
+		 * from xs_ctup.t_self. */
+		if (IsA(scanstate, IndexOnlyScanState))
+		{
+			IndexScanDesc scan = ((IndexOnlyScanState *)scanstate)->ioss_ScanDesc;
+
+			Assert(RelationGetRelid(scan.heapRelation) == table_oid);
+
+			*current_tid = scan->xs_ctup.t_self;
+		}
+		else
+		{
+			/* Use slot_getattr to catch any possible mistakes */
+			tuple_tableoid =
+				DatumGetObjectId(slot_getattr(scanstate->ss_ScanTupleSlot,
+											  TableOidAttributeNumber,
+											  &lisnull));
+			Assert(!lisnull);
+			tuple_tid = (ItemPointer)
+				DatumGetPointer(slot_getattr(scanstate->ss_ScanTupleSlot,
+											 SelfItemPointerAttributeNumber,
+											 &lisnull));
+			Assert(!lisnull);
+
+			Assert(tuple_tableoid == table_oid);
+
+			*current_tid = *tuple_tid;
+		}
 
 		return true;
 	}

--Multipart=_Thu__1_Feb_2018_01_44_44_+0900_ZdDWt_639mx5cmQ9--