Re: next draft of list rewrite - Mailing list pgsql-patches

From Tom Lane
Subject Re: next draft of list rewrite
Date
Msg-id 29405.1075075851@sss.pgh.pa.us
Whole thread Raw
In response to Re: next draft of list rewrite  (Neil Conway <neilc@samurai.com>)
List pgsql-patches
Neil Conway <neilc@samurai.com> writes:
> I thought that once we had decided to change the API, anything was
> fair game: either we need to change every call site of the API (which
> would be required if we adopted ListCell* as the type of the foreach
> iteration variable), or we don't.

Changing the iteration variable type only impacts places that use
foreach, lfirst, and lnext, which is a small subset of the places that
use the complete API.  (A quick look at a couple of planner files
suggested that it'd be about half as many changes, though I'm not
certain that the ones I looked at are very representative.)

A bigger point is that the iteration variable type makes no real
difference in terms of reading the code, whereas a wholesale change in
function names will entail a fair amount of relearning.

BTW, I noticed that lfirst() gets applied to both iteration variables
and true Lists, so we will certainly need to do something about that.
The second case is much less frequent, so possibly
lfirst(first_cell(list)) is the right sort of locution for it.
It might be worthwhile to define ListCell and List as the same struct
temporarily (with some wasted fields in each case) until all the cases
like that can be fixed.

> In addition, there are plenty of infelicities that it would be nice to
> remove from the API. For example:

Yes, there's inconsistency in the names.  That's true all through
Postgres, and is an inevitable result when you have code that's been
worked on by many people over many years.  If we were going to start
enforcing some project-wide naming standards, we could make things
prettier-looking, but I don't see the call to do it to list.c alone.

> I agree that your method is easier procedurally, but ISTM that the
> aggregate amount of work required is about the same: we still need to
> change pretty much every call site of the list API.

You could still do it incrementally, by making macros that implement the
old function names on top of the new.  But I remain of the opinion that
renaming functions simply for the sake of renaming is a dead loss of
time --- and not only your time, but a lot of other contributors'.

> Furthermore, it is quite possible to reduce the pain induced by my
> method. For example, we could create a private CVS branch. In that
> branch, it wouldn't matter if the tree is broken for a week or two, in
> which time we can make the changes across the tree at our leisure, and
> then merge them into HEAD in one relatively painless branch landing.

I doubt the branch landing would be painless, unless the rest of us sit
around twiddling our thumbs meanwhile.  This is such core code that a
wholesale renaming would likely break any patch anyone else has in
progress.

            regards, tom lane

pgsql-patches by date:

Previous
From: Bill Moran
Date:
Subject: [update] Re: Patch to bring \copy syntax more in line with SQL copy
Next
From: Gavin Sherry
Date:
Subject: Clarify libpq docs