Re: Don't overwrite scan key in systable_beginscan()

On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
> When systable_beginscan() and systable_beginscan_ordered() choose an index
> scan, they remap the attribute numbers in the passed-in scan keys to the
> attribute numbers of the index, and then write those remapped attribute
> numbers back into the scan key passed by the caller. This second part is
> surprising and gratuitous. It means that a scan key cannot safely be used
> more than once (but it might sometimes work, depending on circumstances).
> Also, there is no value in providing these remapped attribute numbers back
> to the caller, since they can't do anything with that.
>
> I propose to fix that by making a copy of the scan keys passed by the caller
> and make the modifications there.

No objection, but this would obsolete at least some of these comments (the
catcache.c ones if nothing else):

$ git grep -in scankey | grep -i copy
src/backend/access/gist/gistscan.c:257: * Copy consistent support function to ScanKey structure instead
src/backend/access/gist/gistscan.c:330: * Copy distance support function to ScanKey structure instead of
src/backend/access/nbtree/nbtutils.c:281: ScanKey arrayKeyData; /* modified copy of scan->keyData */
src/backend/access/nbtree/nbtutils.c:3303: * We copy the appropriate indoption value into the scankey sk_flags
src/backend/access/nbtree/nbtutils.c:3318: * It's a bit ugly to modify the caller's copy of the scankey but in practice
src/backend/access/spgist/spgscan.c:385: /* copy scankeys into local storage */
src/backend/utils/cache/catcache.c:1474: * Ok, need to make a lookup in the relation, copy the scankey and
src/backend/utils/cache/catcache.c:1794: * Ok, need to make a lookup in the relation, copy the scankey and
src/backend/utils/cache/relfilenumbermap.c:192: /* copy scankey to local copy, it will be modified during the scan */

> In order to prove to myself that there are no other cases where
> caller-provided scan keys are modified, I went through and const-qualified
> all the APIs. This works out correctly. Several levels down in the stack,
> the access methods make their own copy of the scan keys that they store in
> their scan descriptors, and they use those in non-const-clean ways, but
> that's ok, that's their business. As far as the top-level callers are
> concerned, they can rely on their scan keys to be const after this.

We do still have code mutating IndexScanDescData.keyData, but that's fine. We
must be copying function args to form IndexScanDescData.keyData, or else your
proof patch would have noticed an assignment of const to non-const.