Re: POC: converting Lists into arrays

On Thu, 28 Feb 2019 at 09:26, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I wrote:
> > I did find a number of places where getting rid of explicit lnext()
> > calls led to just plain cleaner code. Most of these were places that
> > could be using forboth() or forthree() and just weren't. There's
> > also several places that are crying out for a forfour() macro, so
> > I'm not sure why we've stubbornly refused to provide it. I'm a bit
> > inclined to just fix those things in the name of code readability,
> > independent of this patch.
>
> 0001 below does this. I found a couple of places that could use
> forfive(), as well. I think this is a clear legibility and
> error-proofing win, and we should just push it.

I've looked over this and I agree that it's a good idea. Reducing the
number of lnext() usages seems like a good idea in order to reduce the
footprint of the main patch.

The only thing of interest that I saw during the review was the fact
that you've chosen to assign colexpr and coldefexpr before the
continue in get_tablefunc(). We may not end up using those values if
we find an ordinality column. I'm pretty sure it's not worth breaking
the mould for that case though, but just noting it anyway.

> > I also noticed that there's quite a lot of places that are testing
> > lnext(cell) for being NULL or not. What that really means is whether
> > this cell is last in the list or not, so maybe readability would be
> > improved by defining a macro along the lines of list_cell_is_last().
> > Any thoughts about that?
>
> 0002 below does this. I'm having a hard time deciding whether this
> part is a good idea or just code churn. It might be more readable
> (especially to newbies) but I can't evaluate that very objectively.
> I'm particularly unsure about whether we need two macros; though the
> way I initially tried it with just list_cell_is_last() seemed kind of
> double-negatively confusing in the places where the test needs to be
> not-last. Also, are these macro names too long, and if so what would
> be better?

I'm less decided on this. Having this now means you'll need to break
the signature of the macro the same way as you'll need to break
lnext(). It's perhaps easier to explain in the release notes about
lnext() having changed so that extension authors can go fix their code
(probably they'll know already from compile failures, but ....). On
the other hand, if the list_cell_is_last() is new, then there will be
no calls to that in extensions anyway. Maybe it's better to do it at
the same time as the List reimplementation to ensure nobody needs to
change anything twice?

> Also: if we accept either or both of these, should we back-patch the
> macro additions, so that these new macros will be available for use
> in back-patched code? I'm not sure that forfour/forfive have enough
> use-cases to worry about that; but the is-last macros would have a
> better case for that, I think.

I see no reason not to put forfour() and forfive() in the back branches.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services