Horton

Thread

Commits

  1. Add for_each_from, to simplify loops starting from non-first list cells.

  2. Minor mop-up for List improvements.

  3. Improve pg_list.h's linitial(), lsecond() and co macros

  1. Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    David Rowley <dgrowleyml@gmail.com> — 2020-09-27T21:08:42Z

    Over in [1] I complained to Tom that I thought he should use
list_nth() instead of linitial() and lsecond(). My reasoning was that
we already knew that the list contained 2 elements, and linitial() and
lsecond() do some additional checks and return NULL if there are fewer
than that amount of elements in the list, or so I thought.  As it
turns out, due to the lfirst() dereferencing the returned pointer,
we'd just segfault if the list being too short and we got NULL.

Since there appears to be no actual safety reason to use those macros
in case your list is too small, it seems better just to use
lfirst(list_nth_cell(..., ...)).  This saves doing the checks to see
if the list is NIL or too short.  It seems nice to get rid of that
additional branching.

We can't just return list_nth() as some code still do things like:

linitial(list) = something;

and we obviously can't assign "something" to the return value of a
function call.

I've attached a patch which improves the macros.

I see on gcc9.3 this reduces the size of the postgres binary by about
16KB: 9027296 to 9011320.

I'm a bit unsure about llast()'s new double evaluation of the list.
Perhaps I can add a new inline function named list_last_cell() to get
around that... Or maybe it doesn't matter. I'm not quite sure what's
best there.

David


[1] https://www.postgresql.org/message-id/CAApHDvqeh8JEqMjpCFTgHD_zu2S03nOVh2srejd+sNLza8M+mg@mail.gmail.com

  2. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-27T23:58:55Z

    David Rowley <dgrowleyml@gmail.com> writes:
> I'm a bit unsure about llast()'s new double evaluation of the list.
> Perhaps I can add a new inline function named list_last_cell() to get
> around that... Or maybe it doesn't matter. I'm not quite sure what's
> best there.

Double evaluation bad, especially in a macro that has not had such a
hazard for the last twenty-plus years.

It might not be worth mucking with llast, as it's not used very heavily
I believe.  But if it is, then let's add another inline function.

			regards, tom lane

  3. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    David Rowley <dgrowleyml@gmail.com> — 2020-09-28T01:50:23Z

    On Mon, 28 Sep 2020 at 12:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I'm a bit unsure about llast()'s new double evaluation of the list.
> > Perhaps I can add a new inline function named list_last_cell() to get
> > around that... Or maybe it doesn't matter. I'm not quite sure what's
> > best there.
>
> Double evaluation bad, especially in a macro that has not had such a
> hazard for the last twenty-plus years.
>
> It might not be worth mucking with llast, as it's not used very heavily
> I believe.  But if it is, then let's add another inline function.

Thanks for having a look at this.

I changed things around to make llast() and the int and oid variant
use a new inline function to get the last cell.

I also pushed the resulting code to master.

David

  4. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T01:58:43Z

    David Rowley <dgrowleyml@gmail.com> writes:
> I changed things around to make llast() and the int and oid variant
> use a new inline function to get the last cell.
> I also pushed the resulting code to master.

LGTM.

			regards, tom lane

  5. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T02:35:06Z

    Poking around to count remaining uses of those inline functions,
I found a few places that should be using the macros instead,
and fixed them.  After that, I notice that list_tail(),
list_third_cell(), and list_fourth_cell() are entirely unreferenced.
I'm hesitant to get rid of list_tail(), because it seems like it
could well be used by extensions.  But I'd bet quite a bit that
list_third_cell() and list_fourth_cell() are not used anywhere
anymore.  Should we get rid of them to shave a few microseconds
from compile times?

			regards, tom lane

  6. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    David Rowley <dgrowleyml@gmail.com> — 2020-09-28T02:47:22Z

    Thanks for 9d299a492.

On Mon, 28 Sep 2020 at 15:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Poking around to count remaining uses of those inline functions,
> I found a few places that should be using the macros instead,
> and fixed them.  After that, I notice that list_tail(),
> list_third_cell(), and list_fourth_cell() are entirely unreferenced.
> I'm hesitant to get rid of list_tail(), because it seems like it
> could well be used by extensions.  But I'd bet quite a bit that
> list_third_cell() and list_fourth_cell() are not used anywhere
> anymore.  Should we get rid of them to shave a few microseconds
> from compile times?

I wouldn't object to the removal of list_third_cell() and list_fourth_cell().

I agree to your reasoning with last_tail(). It does seem more likely
that someone would use it. Although, if you'd proposed to remove it
too, I wouldn't have objected.  It's not like it's hard to reimplement
within an extension for any extensions that use it. Though, perhaps it
would maybe be a shame if that was the sole thing we broke for them
when they try compiling their extension in a year's time on the newly
release PG14.

David

  7. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T15:18:24Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Mon, 28 Sep 2020 at 15:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Poking around to count remaining uses of those inline functions,
>> I found a few places that should be using the macros instead,
>> and fixed them.  After that, I notice that list_tail(),
>> list_third_cell(), and list_fourth_cell() are entirely unreferenced.
>> I'm hesitant to get rid of list_tail(), because it seems like it
>> could well be used by extensions.  But I'd bet quite a bit that
>> list_third_cell() and list_fourth_cell() are not used anywhere
>> anymore.  Should we get rid of them to shave a few microseconds
>> from compile times?

> I wouldn't object to the removal of list_third_cell() and list_fourth_cell().

> I agree to your reasoning with last_tail(). It does seem more likely
> that someone would use it. Although, if you'd proposed to remove it
> too, I wouldn't have objected.  It's not like it's hard to reimplement
> within an extension for any extensions that use it. Though, perhaps it
> would maybe be a shame if that was the sole thing we broke for them
> when they try compiling their extension in a year's time on the newly
> release PG14.

Looking a bit harder, I notice that list_third_cell() and
list_fourth_cell() were in fact introduced in v13, purely to support
lthird() and lfourth().  So it hardly seems likely that any extensions
would have grown direct dependencies on them already.  list_tail()
has been there a long time though.

Just to be sure, I checked codesearch.debian.net, and it failed
to find any outside uses either.  So I'll go ahead and remove
those two.

list_second_cell() does have uses, although I observe that they
are almost exclusively in locutions such as

	for_each_cell(lc, rollups, list_second_cell(rollups))

to iterate over all-but-the-first elements of a list.  I wonder if
we ought to come up with a better notation for that.  I'm imagining
something like

	for_each_from(lc, rollups, 1)

to start from list index 1.  It looks like this would be marginally
more efficient, and perhaps more readable.

			regards, tom lane

  8. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T22:37:52Z

    I wrote:
> list_second_cell() does have uses, although I observe that they
> are almost exclusively in locutions such as
> 	for_each_cell(lc, rollups, list_second_cell(rollups))
> to iterate over all-but-the-first elements of a list.  I wonder if
> we ought to come up with a better notation for that.  I'm imagining
> something like
> 	for_each_from(lc, rollups, 1)
> to start from list index 1.  It looks like this would be marginally
> more efficient, and perhaps more readable.

Concretely, I'm thinking about the attached.  This does seem simpler.
It reduces the size of the executable by about 560 bytes on
my machine, or 46 bytes per usage, which isn't bad.  (Note: this
is in an assert-enabled build, might be different otherwise.)
I didn't try to measure performance changes, but it should be for
the better.

Looking at the remaining instances of for_each_cell, I see several
where it seems like it'd be simpler and clearer to use for_each_from.
But for the moment I confined myself to changing just the instances
following the pattern above.

I noticed while messing with this that I'd neglected to const-ify
the support functions for for_each_cell() and for_both_cell(),
so this fixes that too.

I'm somewhat inclined to back-patch this into v13.  The missing
const decoration seems arguably a bug, which we've missed noticing
only because of our generally lamentable under-usage of const.
And I think it'll be helpful for future back-patching if
for_each_from is available in all versions with the new List API.

			regards, tom lane

  9. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    David Rowley <dgrowleyml@gmail.com> — 2020-09-28T23:26:02Z

    On Tue, 29 Sep 2020 at 11:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > list_second_cell() does have uses, although I observe that they
> > are almost exclusively in locutions such as
> >       for_each_cell(lc, rollups, list_second_cell(rollups))
> > to iterate over all-but-the-first elements of a list.  I wonder if
> > we ought to come up with a better notation for that.  I'm imagining
> > something like
> >       for_each_from(lc, rollups, 1)
> > to start from list index 1.  It looks like this would be marginally
> > more efficient, and perhaps more readable.
>
> Concretely, I'm thinking about the attached.

I had a look over this and I like it.  It seems good as it allows
consumers to choose N programmatically rather than be fixed into using
list_second_cell() or list_fortysecond_cell().

> I'm somewhat inclined to back-patch this into v13.  The missing
> const decoration seems arguably a bug, which we've missed noticing
> only because of our generally lamentable under-usage of const.
> And I think it'll be helpful for future back-patching if
> for_each_from is available in all versions with the new List API.

It does seem fairly low risk and having personally experienced
backpatching pain, I understand your motivation to backpatch.  I
certainly wouldn't object to backpacking but will defer to your better
judgement on whether you choose to or not.

David

  10. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-28T23:42:54Z

    David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, 29 Sep 2020 at 11:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Concretely, I'm thinking about the attached.

> I had a look over this and I like it.  It seems good as it allows
> consumers to choose N programmatically rather than be fixed into using
> list_second_cell() or list_fortysecond_cell().

Thanks for looking!  Right now, if you want to start from a non-first
point in the list, you have to use for_each_cell (or else write out
a loop manually, but let's not).  I'm not proposing to remove that
alternative, but there are surely cases where it's simpler or clearer
to use a list index instead of a ListCell pointer -- especially so if
the list index is a constant.  So I think this is a pretty clear win
that simply failed to occur to me earlier.

>> I'm somewhat inclined to back-patch this into v13.  The missing
>> const decoration seems arguably a bug, which we've missed noticing
>> only because of our generally lamentable under-usage of const.
>> And I think it'll be helpful for future back-patching if
>> for_each_from is available in all versions with the new List API.

> It does seem fairly low risk and having personally experienced
> backpatching pain, I understand your motivation to backpatch.  I
> certainly wouldn't object to backpacking but will defer to your better
> judgement on whether you choose to or not.

A key point here is that everyplace I'm proposing to touch was already
changed in v13 (a fortiori, because list_second_cell wasn't there in v12).
So we can either have two different coding patterns in these areas, or
three.  Two's better from a backpatching standpoint.  The fact that
v13 is barely out the door also factors into this ... a year from now,
my judgment would probably be different.

			regards, tom lane

  11. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    David Rowley <dgrowleyml@gmail.com> — 2020-09-28T23:58:35Z

    On Tue, 29 Sep 2020 at 12:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > It does seem fairly low risk and having personally experienced
> > backpatching pain, I understand your motivation to backpatch.  I
> > certainly wouldn't object to backpacking but will defer to your better
> > judgement on whether you choose to or not.
>
> A key point here is that everyplace I'm proposing to touch was already
> changed in v13 (a fortiori, because list_second_cell wasn't there in v12).
> So we can either have two different coding patterns in these areas, or
> three.  Two's better from a backpatching standpoint.  The fact that
> v13 is barely out the door also factors into this ... a year from now,
> my judgment would probably be different.

Yeah, I understand that part.  The pain I was talking about was
writing a patch for master, trying to apply it to the previous version
only to find it does not apply. After fixing it up for that version
trying to apply that patch to the version before that and getting more
conflicts. Repeating that down to our earliest supported version is
something I'd really love to never have to do again. It's an
exhausting process. It's also risky having to custom write a version
of the patch for each release. So I understand and agree with your
reasoning to backpatch as it could reduce the number of versions of a
patch that must be written to fix a bug.  That could reduce the
chances of someone messing up a backpatch at some later date so might
be a safe option. aka. I'm not going to object to you backpatching
this. :)

David

  12. Re: Small improvements to pg_list.h's linitial(), lsecond(), lthird() etc macros

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-29T00:37:52Z

    David Rowley <dgrowleyml@gmail.com> writes:
> Yeah, I understand that part.  The pain I was talking about was
> writing a patch for master, trying to apply it to the previous version
> only to find it does not apply. After fixing it up for that version
> trying to apply that patch to the version before that and getting more
> conflicts. Repeating that down to our earliest supported version is
> something I'd really love to never have to do again.

Tell me about it :-(.  I've spent countless hours on that sort
of activity.  So I really dislike changing an API and then changing
it again in the next release.

			regards, tom lane