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@leadboat.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)