From: Tom Lane <tgl@sss.pgh.pa.us>
To: Yugo Nagata <nagata@sraoss.co.jp>
cc: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: CURRENT OF causes an error when IndexOnlyScan is used
In-reply-to: <20180315170413.abca0c2b.nagata@sraoss.co.jp>
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>
 <a6ddf99c-6579-e65a-6abd-c0b6259c15c2@postgrespro.ru>
 <13902.1520877384@sss.pgh.pa.us>
 <20180315170413.abca0c2b.nagata@sraoss.co.jp>
Comments: In-reply-to Yugo Nagata <nagata@sraoss.co.jp>
	message dated "Thu, 15 Mar 2018 17:04:13 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <31552.1521234300.1@sss.pgh.pa.us>
Date: Fri, 16 Mar 2018 17:05:00 -0400
Message-ID: <31553.1521234300@sss.pgh.pa.us>
Precedence: bulk

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