Horton

Thread

Commits

  1. Use list_copy_head() instead of list_truncate(list_copy(...), ...)

  2. Tidy up code in get_cheapest_group_keys_order()

  1. Some clean-up work in get_cheapest_group_keys_order()

    David Rowley <dgrowleyml@gmail.com> — 2022-07-12T22:55:07Z

    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

  2. Re: Some clean-up work in get_cheapest_group_keys_order()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-12T23:02:02Z

    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

  3. Re: Some clean-up work in get_cheapest_group_keys_order()

    David Rowley <dgrowleyml@gmail.com> — 2022-07-13T00:50:52Z

    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

  4. Re: Some clean-up work in get_cheapest_group_keys_order()

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-13T01:12:06Z

    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

  5. Re: Some clean-up work in get_cheapest_group_keys_order()

    David Rowley <dgrowleyml@gmail.com> — 2022-07-13T03:06:10Z

    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