Re: nitpick about poor style in MergeAttributes

On Wed, May 22, 2019 at 10:21 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, May 22, 2019 at 06:20:01PM -0700, Mark Dilger wrote:
> > What to do about this is harder to say. In the following
> > patch, I'm just doing what I think is standard for callers
> > of list_delete_cell, and assigning the return value back
> > to the list (similar to how a call to repalloc should do).
> > But since there is an implicit assumption that the list
> > is never emptied by this operation, perhaps checking
> > against NIL and elog'ing makes more sense?
>
> Yes, I agree that this is a bit fuzzy, and this code is new as of
> 705d433. As you say, I agree that making sure that the return value
> of list_delete_cell is not NIL is a sensible choice.
>
> I don't think that an elog() is in place here though as this does not
> rely directly on catalog contents, what about just an assertion?

I think assigning the return value (as I did in my small patch) and
then asserting that 'schema' is not NIL would be good.

> Here is an idea of message for the elog(ERROR) if we go that way:
> "no remaining columns after merging column \"%s\"".

Perhaps. I like your idea of adding an assertion better.

mark