Обсуждение: [PATCH] Check that index can return in get_actual_variable_range()

Hi all,

Some recent changes were made to remove the explicit dependency on btree indexes in some parts of the code. One of
thesechanges was made in commit 9ef1851685b, which allows non-btree indexes to be used in get_actual_variable_range().
Afollow-up commit ee1ae8b99f9 fixes the cases where an index doesn’t have a sortopfamily as this is a prerequisite to
beused in get_actual_variable_range(). However, I found out recently that indices that have ‘amcanorder = true’ but do
notallow index-only-scans (amcanreturn returns false or is NULL) will pass all of the conditions, while they should be
rejectedsince 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-onlyscans. 

Regards,
Maxime Schoemans

Hi Maxime,

> Some recent changes were made to remove the explicit dependency on btree indexes in some parts of the code. One of
thesechanges was made in commit 9ef1851685b, which allows non-btree indexes to be used in get_actual_variable_range().
Afollow-up commit ee1ae8b99f9 fixes the cases where an index doesn’t have a sortopfamily as this is a prerequisite to
beused in get_actual_variable_range(). However, I found out recently that indices that have ‘amcanorder = true’ but do
notallow index-only-scans (amcanreturn returns false or is NULL) will pass all of the conditions, while they should be
rejectedsince 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-onlyscans. 

Thanks for the patch.

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

--
Best regards,
Aleksander Alekseev

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
atest 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
specificallysets 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
wouldpass. 

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
falseand 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
withthe 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
choosingthe 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/falsenot a valid choice for an index?  If the latter, shouldn't there be a check for that somewhere,
orat least an Assert()? 

After a short offline discussion with Mark, we determined that this issue was due to treebproperty still returning true
forthe 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
havea 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
specificallysets 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
wouldpass. 

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