[PATCH] Check that index can return in get_actual_variable_range()

Hi Maxime,

Some recent changes were made to remove the explicit dependency on btree indexes in some parts of the code. One of these changes was made in commit 9ef1851685b, which allows non-btree indexes to be used in get_actual_variable_range(). A follow-up commit ee1ae8b99f9 fixes the cases where an index doesn’t have a sortopfamily as this is a prerequisite to be used in get_actual_variable_range(). However, I found out recently that indices that have ‘amcanorder = true’ but do not allow index-only-scans (amcanreturn returns false or is NULL) will pass all of the conditions, while they should be rejected since get_actual_variable_range() uses the index-only-scan machinery in get_actual_variable_endpoint().

Attached is a small patch that adds a check in get_actual_variable_range() to reject indices that do not allow index-only scans.

Thanks for the patch.

Can you think of any test cases we can add to the code base?

--
Best regards,
Aleksander Alekseev

Testing with the src/test/modules/treeb work in the patch series at [1]/messages/by-id/5083d9ed-837d-47cd-9d40-fded88b62c10@eisentraut.org,
modifying treebcanreturn() to always return false and modifying
_treeb_first(), _treeb_next(), and _treeb_endpoint() to set scan->xs_itup
to NULL rather than to a tuple, without the patch there are a number of
test failures with "ERROR: no data returned for index-only scan", but with
the patch applied those errors still appear. They are raised by
nodeIndexonlyscan.c at line 213, inside IndexOnlyNext(), suggesting that
changing treebcanreturn() to return false was not enough to dissuade the
planner from choosing the index for an index only scan.

Is there a bug in how the system selects an index for an index-only scan?
Or is the combination amcanorder=true && amcanreturn=NULL/false not a valid
choice for an index? If the latter, shouldn't there be a check for that
somewhere, or at least an Assert()?

[1]: /messages/by-id/5083d9ed-837d-47cd-9d40-fded88b62c10@eisentraut.org
/messages/by-id/5083d9ed-837d-47cd-9d40-fded88b62c10@eisentraut.org

On Thu, Sep 18, 2025 at 5:32 AM Aleksander Alekseev <
aleksander@tigerdata.com> wrote:

Hi Maxime,

Some recent changes were made to remove the explicit dependency on btree

indexes in some parts of the code. One of these changes was made in commit
9ef1851685b, which allows non-btree indexes to be used in
get_actual_variable_range(). A follow-up commit ee1ae8b99f9 fixes the cases
where an index doesn’t have a sortopfamily as this is a prerequisite to be
used in get_actual_variable_range(). However, I found out recently that
indices that have ‘amcanorder = true’ but do not allow index-only-scans
(amcanreturn returns false or is NULL) will pass all of the conditions,
while they should be rejected since get_actual_variable_range() uses the
index-only-scan machinery in get_actual_variable_endpoint().

Attached is a small patch that adds a check in

get_actual_variable_range() to reject indices that do not allow index-only
scans.

Thanks for the patch.

Can you think of any test cases we can add to the code base?

--
Best regards,
Aleksander Alekseev

--

*Mark Dilger*

On 18 Sep 2025, at 14:31, Aleksander Alekseev <aleksander@tigerdata.com> wrote:

Can you think of any test cases we can add to the code base?

The only idea I have would be adding a new index module in src/test/modules that has this particular property and have a test that will hit get_actual_variable_range().
For example, it could be a new access method that is a copy of btree, but whose amcanreturn property is null and that specifically sets scan->xs_itup to null in amgettuple.

Without the patch, such a test would fail with 'ERROR: no data returned for index-only scan’, while with the patch it would pass.

Best,
Maxime Schoemans

On 18 Sep 2025, at 15:36, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

Testing with the src/test/modules/treeb work in the patch series at [1], modifying treebcanreturn() to always return false and modifying _treeb_first(), _treeb_next(), and _treeb_endpoint() to set scan->xs_itup to NULL rather than to a tuple, without the patch there are a number of test failures with "ERROR: no data returned for index-only scan", but with the patch applied those errors still appear. They are raised by nodeIndexonlyscan.c at line 213, inside IndexOnlyNext(), suggesting that changing treebcanreturn() to return false was not enough to dissuade the planner from choosing the index for an index only scan.

Is there a bug in how the system selects an index for an index-only scan? Or is the combination amcanorder=true && amcanreturn=NULL/false not a valid choice for an index? If the latter, shouldn't there be a check for that somewhere, or at least an Assert()?

After a short offline discussion with Mark, we determined that this issue was due to treebproperty still returning true for the AMPROP_RETURNABLE property. This was a small oversight in the testing, but does not indicate a bug in postgres.

Hi Maxime,

Can you think of any test cases we can add to the code base?

The only idea I have would be adding a new index module in src/test/modules that has this particular property and have a test that will hit get_actual_variable_range().
For example, it could be a new access method that is a copy of btree, but whose amcanreturn property is null and that specifically sets scan->xs_itup to null in amgettuple.

Without the patch, such a test would fail with 'ERROR: no data returned for index-only scan’, while with the patch it would pass.

Yes, this is how we typically test cases like this. IMO adding a test
module would be helpful. It can be reused for other scenarios.

--
Best regards,
Aleksander Alekseev

On 19 Sep 2025, at 10:20, Aleksander Alekseev <aleksander@tigerdata.com>
wrote:

Yes, this is how we typically test cases like this. IMO adding a test
module would be helpful. It can be reused for other scenarios.

Here is an updated patch set.
- 0001 is unchanged.
- 0002 contains the module that tests the correct behavior of
get_actual_variable_range for non-returning ordering indices.
It contains a copy of the btree handler function with its index-only
capabilities removed. If you apply patch 0002 on master without 0001,
you will see that the test returns an error (ERROR: no data returned
for index-only scan) as it tries to use the index in
get_actual_variable_range, which shouldn’t be the case.

Best,
Maxime Schoemans

On 22.09.25 16:38, Maxime Schoemans wrote:

On 19 Sep 2025, at 10:20, Aleksander Alekseev <aleksander@tigerdata.com>
wrote:

Yes, this is how we typically test cases like this. IMO adding a test
module would be helpful. It can be reused for other scenarios.

Here is an updated patch set.
- 0001 is unchanged.
- 0002 contains the module that tests the correct behavior of
get_actual_variable_range for non-returning ordering indices.
It contains a copy of the btree handler function with its index-only
capabilities removed. If you apply patch 0002 on master without 0001,
you will see that the test returns an error (ERROR: no data returned
for index-only scan) as it tries to use the index in
get_actual_variable_range, which shouldn’t be the case.

I have committed the first patch.

The test suite is probably a bit too bulky for testing this particular
niche behavior. Also, it doesn't work with assertions enabled because
of the hardcoded BTREE_AM_OID in src/include/access/nbtree.h. So I
don't plan to commit that. But it's good to have it in the archive, and
perhaps it can be part of a larger test suite for the index AM API at
some point in the future.

On 28 Oct 2025, at 10:20, Peter Eisentraut <peter@eisentraut.org> wrote:

I have committed the first patch.

Great, thank you for looking at this.

The test suite is probably a bit too bulky for testing this particular niche behavior. Also, it doesn't work with assertions enabled because of the hardcoded BTREE_AM_OID in src/include/access/nbtree.h. So I don't plan to commit that. But it's good to have it in the archive, and perhaps it can be part of a larger test suite for the index AM API at some point in the future.

And I agree, the test suite is probably overkill for this two-line patch, but it might be good to have around for later.