Re: [BUGFIX] amcanbackward is not checked before building backward index paths

On Wed, May 16, 2018 at 1:41 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> writes:
> > Experimenting with the new pluggable storage API, I found that
> amcanbackward
> > flag is not checked in build_index_paths() before
> > build_index_pathkeys(... BackwardScanDirection) call when we are building
> > paths for ORDER BY. And this flag is even not copied into IndexOptInfo
> struct.
> > Obviously, this can lead to misuse of Backward Index [Only] Scan plans.
>
> > Attached patch with the corresponding fix.
>
> I think this patch is based on a misunderstanding of what amcanbackward
> means. We *require* indexes that claim to support amcanorder to support
> scanning in either direction. What amcanbackward is about is whether
> the index can support reversing direction mid-scan, as would be required
> to support FETCH FORWARD followed by FETCH BACKWARD in a cursor. That's
> actually independent of whether the index can implement a defined
> ordering; see for example the hash AM, which sets amcanbackward but not
> amcanorder.
>
> This is documented, though perhaps not sufficiently clearly, in
> indexam.sgml:
>
> The amgettuple function has a direction argument, which can be either
> ForwardScanDirection (the normal case) or BackwardScanDirection. If
> the first call after amrescan specifies BackwardScanDirection, then
> the set of matching index entries is to be scanned back-to-front
> rather than in the normal front-to-back direction, so amgettuple must
> return the last matching tuple in the index, rather than the first one
> as it normally would. (This will only occur for access methods that
> set amcanorder to true.) After the first call, amgettuple must be
> prepared to advance the scan in either direction from the most
> recently returned entry. (But if amcanbackward is false, all
> subsequent calls will have the same direction as the first one.)
>

Thank you for clarifying this point. We've missed that.

Perhaps there is a case for adding an additional flag to allow specifying
> "I can't support ORDER BY DESC", but I'm not in a big hurry to do so.
> I think there would be more changes than this needed to handle such a
> restriction, anyway.
>

OK, got it. We'll probably propose a patch implementing that to the
next commitfest.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company