Thread: CURRENT OF causes an error when IndexOnlyScan is used
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>
Attachment
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>
Attachment
Yugo Nagata <nagata@sraoss.co.jp> writes:
> I'm sorry the patch attached in the previous mail is broken and
> not raises a compile error. I attached the fixed patch.
This patch is almost certainly wrong: you can't assume that the scan-level
state matches the tuple we are currently processing at top level. Any
sort of delaying action, for instance a sort or materialize node in
between, would break it.
We need to either fix this aspect:
>> IndexOnlyScan returns a virtual tuple that doesn't have system
>> column, so we can not get ctid in the same way of other plans.
or else disallow using IndexOnlyScan when the ctid is needed.
regards, tom lane
On Wed, 31 Jan 2018 21:12:51 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Yugo Nagata <nagata@sraoss.co.jp> writes: > > I'm sorry the patch attached in the previous mail is broken and > > not raises a compile error. I attached the fixed patch. > > This patch is almost certainly wrong: you can't assume that the scan-level > state matches the tuple we are currently processing at top level. Any > sort of delaying action, for instance a sort or materialize node in > between, would break it. In execCurrentOf(), when FOR UPDATE is not used, search_plan_tree() searches through the PlanState tree for a scan node and if a sort or materialize node (for example) is found it fails with the following error. ERROR cursor xxx is not a simply updatable scan of table yyy So, I think what you concern would not occur by the patch as well as the orginal code. However, I may be missing something. Could you explain more about this if so? > > We need to either fix this aspect: > > >> IndexOnlyScan returns a virtual tuple that doesn't have system > >> column, so we can not get ctid in the same way of other plans. > > or else disallow using IndexOnlyScan when the ctid is needed. CURRENT OF is used after the scan is executed and a tuple is fetched, so we can't know whether the ctid is needed or not in advance in this case. We can raise an error message when CURRENT OF is used for IndexOnlyScan plan, though. Regards, > > regards, tom lane -- Yugo Nagata <nagata@sraoss.co.jp>
01.02.2018 05:12, Tom Lane: > Yugo Nagata <nagata@sraoss.co.jp> writes: >> I'm sorry the patch attached in the previous mail is broken and >> not raises a compile error. I attached the fixed patch. > This patch is almost certainly wrong: you can't assume that the scan-level > state matches the tuple we are currently processing at top level. Any > sort of delaying action, for instance a sort or materialize node in > between, would break it. > > We need to either fix this aspect: > >>> IndexOnlyScan returns a virtual tuple that doesn't have system >>> column, so we can not get ctid in the same way of other plans. 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/ -- Anastasia Lubennikova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Attachment
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? -- Best regards, Aleksander Alekseev
Attachment
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
Attachment
Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:
> [ return_heaptuple_in_btree_indexonlyscan_v2.patch ]
I took a quick look at this, but I'm concerned about a couple of points:
1. What's the performance penalty of forming (and then deforming) the
added heap tuple? We'd be paying it for every index-only scan, whether
or not any CURRENT OF fetch ever happened.
2. The constructed tuple provides tableoid and ctid all right, but it'd
still have garbage values for other system columns. As the code stands,
we will properly error out if some attempt is made to fetch any of those
other columns, but with this change we'd just return the garbage. This
is a minor point, but it doesn't seem negligible to me; it might've been
hard to identify the bug at hand if we'd not had the cross-check of not
building a heap tuple.
At this point Yugo-san's original patch is starting to look more
attractive. It's still ugly, but at least it's not imposing a performance
cost on unrelated queries.
regards, tom lane
On Mon, 12 Mar 2018 13:56:24 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes: > > [ return_heaptuple_in_btree_indexonlyscan_v2.patch ] > > I took a quick look at this, but I'm concerned about a couple of points: > > 1. What's the performance penalty of forming (and then deforming) the > added heap tuple? We'd be paying it for every index-only scan, whether > or not any CURRENT OF fetch ever happened. > > 2. The constructed tuple provides tableoid and ctid all right, but it'd > still have garbage values for other system columns. As the code stands, > we will properly error out if some attempt is made to fetch any of those > other columns, but with this change we'd just return the garbage. This > is a minor point, but it doesn't seem negligible to me; it might've been > hard to identify the bug at hand if we'd not had the cross-check of not > building a heap tuple. In addition, this patch fixes only nbtree indexes, but the simillar problem will occur also on GIST indexes which support index-only scan. If we resolve this bug by fixing index access methods, I think we need to fix all existing indexes that support index-only scan and also describe the specification in the documents, comments, or README, etc. for future indexes. > At this point Yugo-san's original patch is starting to look more > attractive. It's still ugly, but at least it's not imposing a performance > cost on unrelated queries. I would like to elaborate my patch if needed and possible. Any suggestion would be appriciated. Thanks, > > regards, tom lane -- Yugo Nagata <nagata@sraoss.co.jp>
Yugo Nagata <nagata@sraoss.co.jp> writes:
> On Mon, 12 Mar 2018 13:56:24 -0400
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I took a quick look at this, but I'm concerned about a couple of points:
> In addition, this patch fixes only nbtree indexes, but the simillar problem
> will occur also on GIST indexes which support index-only scan. If we resolve
> this bug by fixing index access methods, I think we need to fix all existing
> indexes that support index-only scan and also describe the specification in
> the documents, comments, or README, etc. for future indexes.
Yeah, that's a pretty good point.
>> At this point Yugo-san's original patch is starting to look more
>> attractive. It's still ugly, but at least it's not imposing a performance
>> cost on unrelated queries.
> I would like to elaborate my patch if needed and possible. Any suggestion
> would be appriciated.
I don't think there's much else to be done so far as the IndexOnlyScan
case is concerned. However, I notice that somebody's made
search_plan_tree accept ForeignScanState and CustomScanState as possible
matches for WHERE CURRENT OF, and I find that rather troubling. It seems
likely to me that both of those would have the same problem as
IndexOnlyScans, ie whatever they're returning is probably a virtual tuple
without any ctid field. So we'd get the same unfriendly failure as you
complained of originally.
I wonder whether it wouldn't be a good idea to provide a way for an FDW
or CustomScan provider to return a TID, or at least give a more polite
FEATURE_NOT_SUPPORTED error than what happens now. However, that seems
more like a new feature than a bug fix; certainly any extension of those
APIs is something we'd not back-patch.
In the meantime, we could fix execCurrent.c so that it throws
FEATURE_NOT_SUPPORTED rather than the current failure if the slot it's
looking at doesn't contain a physical tuple.
regards, tom lane
I wrote:
> In the meantime, we could fix execCurrent.c so that it throws
> FEATURE_NOT_SUPPORTED rather than the current failure if the slot it's
> looking at doesn't contain a physical tuple.
Concretely, I think we should do the attached, so as to cover any other
situations where the scan type doesn't return a physical tuple. I kept
it separate from your patch so it's easy to test (the original case you
gave now throws the appropriate error).
The existing error when execCurrentOf can't figure out what to do with
the plan is ERRCODE_INVALID_CURSOR_STATE with message
"cursor \"%s\" is not a simply updatable scan of table \"%s\""
so for this draft patch I just used that. I'm not sure if it would be
a better idea to throw a different SQLSTATE or different error text
for this case. Any thoughts on that?
regards, tom lane
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index a2f67f2..c45a488 100644
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
*************** slot_attisnull(TupleTableSlot *slot, int
*** 1367,1372 ****
--- 1367,1398 ----
}
/*
+ * 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
*/
void
diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c
index ce7d4ac..2296c9b 100644
*** a/src/backend/executor/execCurrent.c
--- b/src/backend/executor/execCurrent.c
*************** execCurrentOf(CurrentOfExpr *cexpr,
*** 150,157 ****
else
{
ScanState *scanstate;
bool lisnull;
- Oid tuple_tableoid PG_USED_FOR_ASSERTS_ONLY;
ItemPointer tuple_tid;
/*
--- 150,157 ----
else
{
ScanState *scanstate;
+ Datum ldatum;
bool lisnull;
ItemPointer tuple_tid;
/*
*************** execCurrentOf(CurrentOfExpr *cexpr,
*** 183,201 ****
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;
--- 183,213 ----
if (TupIsNull(scanstate->ss_ScanTupleSlot))
return false;
! /*
! * Try to fetch tableoid and CTID from the scan node's current tuple.
! * If the scan type hasn't provided a physical tuple, we have to fail.
! */
! 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);
! 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;
diff --git a/src/include/executor/tuptable.h b/src/include/executor/tuptable.h
index 0642a3a..a5779b1 100644
*** a/src/include/executor/tuptable.h
--- b/src/include/executor/tuptable.h
*************** extern Datum slot_getattr(TupleTableSlot
*** 170,174 ****
--- 170,176 ----
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 */