Re: nitpick about poor style in MergeAttributes

On Thu, May 23, 2019 at 5:24 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Thu, May 23, 2019 at 08:27:19AM -0700, Mark Dilger wrote:
> > On Thu, May 23, 2019 at 7:54 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Are we sure that's not just a newly-introduced bug, ie it has not
> >> been tested in cases where the tlist could become empty? My first
> >> thought would be to assign the list pointer value back as per usual
> >> coding convention, not to double down on the assumption that this
> >> was well-considered code.
> >
> > I don't think that is disputed. I was debating between assigning
> > it back and also asserting that it is not NIL vs. assigning it back
> > and elog/ereporting if it is NIL. Of course, this is assuming the
> > code was designed with the expectation that the list can never
> > become empty. If you think it might become empty, and that the
> > surrounding code can handle that sensibly, then perhaps we
> > need neither the assertion nor the elog/ereport, though we still
> > need the assignment.
>
> Looking closer, this code is not new as of v12. We have that since
> e7b3349 which has introduced CREATE TABLE OF. Anyway, I think that
> assigning the result of list_delete_cell and adding an assertion like
> in the attached are saner things to do. This code scans each entry in
> the list and removes columns with duplicate names, so we should never
> finish with an empty list as we will in the first case always merge
> down to at least one column. That's rather a nit, but I guess that
> this is better than the previous code which assumed that silently?

I like it better because it makes static analysis of the code easier,
and because if anybody ever changed list_delete_cell to return a
different list object in more cases than just when the list is completely
empty, this call site would be silently wrong.

Thanks for the patch!

mark