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

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

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

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

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

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

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

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

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

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

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

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