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?
--
Michael