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

On 12.08.24 09:44, Peter Eisentraut wrote:
> On 09.08.24 06:55, Tom Lane wrote:
>> Noah Misch <noah(at)leadboat(dot)com> writes:
>>> On Thu, Aug 08, 2024 at 08:46:35AM +0200, Peter Eisentraut wrote:
>>>> 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):
>>
>> That ties into something I forgot to ask: aren't there any copy steps
>> or other overhead that we could remove, given this new API constraint?
>> That would help address Robert's concern.
>
> I added two more patches to the series here.
>
> First (or 0004), some additional cleanup for code that had to workaround
> systable_beginscan() overwriting the scan keys, along the lines
> suggested by Noah.
>
> Second (or 0005), an alternative to palloc is to make the converted scan
> keys a normal local variable.  Then it's just a question of whether a
> smaller palloc is preferred over an over-allocated local variable.  I
> think I still prefer the palloc version, but it doesn't matter much
> either way I think.

Looks like the discussion here has settled, so I plan to go head with
patches

[PATCH v2 1/5] Don't overwrite scan key in systable_beginscan()
[PATCH v2 3/5] Replace gratuitous memmove() with memcpy()
[PATCH v2 4/5] Update some code that handled systable_beginscan()
overwriting scan key

(folding patch 4 into patch 1)