Fix WHERE CURRENT OF when the referenced cursor uses an index-only scan.

"UPDATE/DELETE WHERE CURRENT OF cursor_name" failed, with an error message
like "cannot extract system attribute from virtual tuple", if the cursor
was using a index-only scan for the target table.  Fix it by digging the
current TID out of the indexscan state.

It seems likely that the same failure could occur for CustomScan plans
and perhaps some FDW plan types, so that leaving this to be treated as an
internal error with an obscure message isn't as good an idea as it first
seemed.  Hence, add a bit of heaptuple.c infrastructure to let us deliver
a more on-topic message.  I chose to make the message match what you get
for the case where execCurrentOf can't identify the target scan node at
all, "cursor "foo" is not a simply updatable scan of table "bar"".
Perhaps it should be different, but we can always adjust that later.

In the future, it might be nice to provide hooks that would let custom
scan providers and/or FDWs deal with this in other ways; but that's
not a suitable topic for a back-patchable bug fix.

It's been like this all along, so back-patch to all supported branches.

Yugo Nagata and Tom Lane

Discussion: https://postgr.es/m/20180201013349.937dfc5f.nagata@sraoss.co.jp

diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index a2f67f2332c2808bff2afbcc0fb3b5ad62f9b1fc..c45a48812bf9a34caee9f3be81a6e3e369c47ea6 100644 (file)
--- a/src/backend/access/common/heaptuple.c
+++ b/src/backend/access/common/heaptuple.c
@@ -1366,6 +1366,32 @@ slot_attisnull(TupleTableSlot *slot, int attnum)
        return heap_attisnull(tuple, attnum);
 }
 
+/*
+ * slot_getsysattr
+ *             This function fetches a system attribute of the slot's current tuple.
+ *             Unlike slot_getattr, if the slot does not contain system attributes,
+ *             this will return false (with a NULL attribute value) instead of
+ *             throwing an error.
+ */
+bool
+slot_getsysattr(TupleTableSlot *slot, int attnum,
+                               Datum *value, bool *isnull)
+{
+       HeapTuple       tuple = slot->tts_tuple;
+
+       Assert(attnum < 0);                     /* else caller error */
+       if (tuple == NULL ||
+               tuple == &(slot->tts_minhdr))
+       {
+               /* No physical tuple, or minimal tuple, so fail */
+               *value = (Datum) 0;
+               *isnull = true;
+               return false;
+       }
+       *value = heap_getsysattr(tuple, attnum, slot->tts_tupleDescriptor, isnull);
+       return true;
+}
+
 /*
  * heap_freetuple
  */

diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac592af30020620df8e68bfca16f8f398b0..f70e54fe2af1ec832909544d4842df64440cef1e 100644 (file)
--- a/src/backend/executor/execCurrent.c
+++ b/src/backend/executor/execCurrent.c
@@ -12,6 +12,7 @@
  */
 #include "postgres.h"
 
+#include "access/relscan.h"
 #include "access/sysattr.h"
 #include "catalog/pg_type.h"
 #include "executor/executor.h"
@@ -149,16 +150,13 @@ execCurrentOf(CurrentOfExpr *cexpr,
        }
        else
        {
-               ScanState  *scanstate;
-               bool            lisnull;
-               Oid                     tuple_tableoid PG_USED_FOR_ASSERTS_ONLY;
-               ItemPointer tuple_tid;
-
                /*
                 * Without FOR UPDATE, we dig through the cursor's plan to find the
                 * scan node.  Fail if it's not there or buried underneath
                 * aggregation.
                 */
+               ScanState  *scanstate;
+
                scanstate = search_plan_tree(queryDesc->planstate, table_oid);
                if (!scanstate)
                        ereport(ERROR,
@@ -183,21 +181,62 @@ 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;
+               /*
+                * Extract TID of the scan's current row.  The mechanism for this is
+                * in principle scan-type-dependent, but for most scan types, we can
+                * just dig the TID out of the physical scan tuple.
+                */
+               if (IsA(scanstate, IndexOnlyScanState))
+               {
+                       /*
+                        * For IndexOnlyScan, the tuple stored in ss_ScanTupleSlot may be
+                        * a virtual tuple that does not have the ctid column, so we have
+                        * to get the TID from xs_ctup.t_self.
+                        */
+                       IndexScanDesc scan = ((IndexOnlyScanState *) scanstate)->ioss_ScanDesc;
+
+                       *current_tid = scan->xs_ctup.t_self;
+               }
+               else
+               {
+                       /*
+                        * Default case: try to fetch TID from the scan node's current
+                        * tuple.  As an extra cross-check, verify tableoid in the current
+                        * tuple.  If the scan hasn't provided a physical tuple, we have
+                        * to fail.
+                        */
+                       Datum           ldatum;
+                       bool            lisnull;
+                       ItemPointer tuple_tid;
+
+#ifdef USE_ASSERT_CHECKING
+                       if (!slot_getsysattr(scanstate->ss_ScanTupleSlot,
+                                                                TableOidAttributeNumber,
+                                                                &ldatum,
+                                                                &lisnull))
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_INVALID_CURSOR_STATE),
+                                                errmsg("cursor \"%s\" is not a simply updatable scan of table \"%s\"",
+                                                               cursor_name, table_name)));
+                       Assert(!lisnull);
+                       Assert(DatumGetObjectId(ldatum) == table_oid);
+#endif
+
+                       if (!slot_getsysattr(scanstate->ss_ScanTupleSlot,
+                                                                SelfItemPointerAttributeNumber,
+                                                                &ldatum,
+                                                                &lisnull))
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_INVALID_CURSOR_STATE),
+                                                errmsg("cursor \"%s\" is not a simply updatable scan of table \"%s\"",
+                                                               cursor_name, table_name)));
+                       Assert(!lisnull);
+                       tuple_tid = (ItemPointer) DatumGetPointer(ldatum);
+
+                       *current_tid = *tuple_tid;
+               }
+
+               Assert(ItemPointerIsValid(current_tid));
 
                return true;
        }

diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 0642a3ada5e9670570235da0bc1ce7c0c5c4ce81..a5779b15eab6d5e03b8f0f1019d7785e48d1a6f1 100644 (file)
--- a/src/include/executor/tuptable.h
+++ b/src/include/executor/tuptable.h
@@ -170,5 +170,7 @@ extern Datum slot_getattr(TupleTableSlot *slot, int attnum, bool *isnull);
 extern void slot_getallattrs(TupleTableSlot *slot);
 extern void slot_getsomeattrs(TupleTableSlot *slot, int attnum);
 extern bool slot_attisnull(TupleTableSlot *slot, int attnum);
+extern bool slot_getsysattr(TupleTableSlot *slot, int attnum,
+                               Datum *value, bool *isnull);
 
 #endif                                                 /* TUPTABLE_H */

diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index 1b8f7b69d1d9604e343d3b1fd7273b03a957a45d..048b2fc3e3a0d064fca7cc075fd7c7ce2e955368 100644 (file)
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -1245,6 +1245,30 @@ FETCH FROM c1;
 
 DELETE FROM ucview WHERE CURRENT OF c1; -- fail, views not supported
 ERROR:  WHERE CURRENT OF on a view is not implemented
+ROLLBACK;
+-- Check WHERE CURRENT OF with an index-only scan
+BEGIN;
+EXPLAIN (costs off)
+DECLARE c1 CURSOR FOR SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+                 QUERY PLAN                  
+---------------------------------------------
+ Index Only Scan using onek_stringu1 on onek
+   Index Cond: (stringu1 = 'DZAAAA'::name)
+(2 rows)
+
+DECLARE c1 CURSOR FOR SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+FETCH FROM c1;
+ stringu1 
+----------
+ DZAAAA
+(1 row)
+
+DELETE FROM onek WHERE CURRENT OF c1;
+SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+ stringu1 
+----------
+(0 rows)
+
 ROLLBACK;
 -- Make sure snapshot management works okay, per bug report in
 -- 235395b90909301035v7228ce63q392931f15aa74b31@mail.gmail.com

diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index a1c812e937f78a5d45974d97ee7f9233af0dc6c0..d1a589094eadefbce03203596aad7add81f80d42 100644 (file)
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -462,6 +462,16 @@ FETCH FROM c1;
 DELETE FROM ucview WHERE CURRENT OF c1; -- fail, views not supported
 ROLLBACK;
 
+-- Check WHERE CURRENT OF with an index-only scan
+BEGIN;
+EXPLAIN (costs off)
+DECLARE c1 CURSOR FOR SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+DECLARE c1 CURSOR FOR SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+FETCH FROM c1;
+DELETE FROM onek WHERE CURRENT OF c1;
+SELECT stringu1 FROM onek WHERE stringu1 = 'DZAAAA';
+ROLLBACK;
+
 -- Make sure snapshot management works okay, per bug report in
 -- 235395b90909301035v7228ce63q392931f15aa74b31@mail.gmail.com
 BEGIN;