Thread: Some clean-up work in get_cheapest_group_keys_order()
I was rebasing a patch which requires me to make some changes in get_cheapest_group_keys_order(). I noticed a few things in there that I think we could do a little better on: * The code uses pfree() on a list and it should be using list_free() * There's a manually coded for loop over a list which seems to be done so we can skip the first n elements of the list. for_each_from() should be used for that. * I think list_truncate(list_copy(list), n) is a pretty bad way to copy the first n elements of a list, especially when n is likely to be 0 most of the time. I think we should just add a function called list_copy_head(). We already have list_copy_tail(). * We could reduce some of the branching in the while loop and just set cheapest_sort_cost to DBL_MAX to save having to check if we're doing the first loop. I think the first 3 are worth fixing in PG15 since all that code is new to that version. The 4th, I'm so sure about. Does anyone else have any thoughts? David
Attachment
David Rowley <dgrowleyml@gmail.com> writes:
> * I think list_truncate(list_copy(list), n) is a pretty bad way to
> copy the first n elements of a list, especially when n is likely to be
> 0 most of the time. I think we should just add a function called
> list_copy_head(). We already have list_copy_tail().
Agreed, but I think there are other instances of that idiom that
should be cleaned up while you're at it.
> I think the first 3 are worth fixing in PG15 since all that code is
> new to that version. The 4th, I'm so sure about.
I'd say keeping v15 and v16 in sync here is worth something.
regards, tom lane
On Wed, 13 Jul 2022 at 11:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <dgrowleyml@gmail.com> writes: > > * I think list_truncate(list_copy(list), n) is a pretty bad way to > > copy the first n elements of a list, especially when n is likely to be > > 0 most of the time. I think we should just add a function called > > list_copy_head(). We already have list_copy_tail(). > > Agreed, but I think there are other instances of that idiom that > should be cleaned up while you're at it. Agreed. I imagine we should just do the remaining cleanup in master only. Do you agree? David
David Rowley <dgrowleyml@gmail.com> writes:
> On Wed, 13 Jul 2022 at 11:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Agreed, but I think there are other instances of that idiom that
>> should be cleaned up while you're at it.
> Agreed. I imagine we should just do the remaining cleanup in master
> only. Do you agree?
No objection.
regards, tom lane
On Wed, 13 Jul 2022 at 13:12, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <dgrowleyml@gmail.com> writes: > > On Wed, 13 Jul 2022 at 11:02, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Agreed, but I think there are other instances of that idiom that > >> should be cleaned up while you're at it. > > > Agreed. I imagine we should just do the remaining cleanup in master > > only. Do you agree? > > No objection. I've now pushed the original patch to 15 and master and also pushed a cleanup commit to remove the list_truncate(list_copy instances from master only. Thanks for looking. David