On Thu, May 23, 2019 at 5:24 PM Michael Paquier <mich...@paquier.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 <t...@sss.pgh.pa.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