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

Nikita Glukhov <n.gluhov@postgrespro.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.)

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.