On Thu, Mar 30, 2023 at 11:12 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > > > > 5. > > > AFAIK, the "if (change->data.tp.oldtuple)" can only be true for UPDATE > > > or DELETE, so the code would be better to include a sanity Assert. > > > > > > SUGGESTION > > > if (change->data.tp.oldtuple) > > > { > > > Assert(action == REORDER_BUFFER_CHANGE_UPDATE || action == > > > REORDER_BUFFER_CHANGE_DELETE); > > > ... > > > > > > > It might be fine but I am not sure if it's necessary to add this in this > > patch as we don't have such assertion before. > > The Asserts are just for sanity and self-documentation regarding what > actions can get into this logic. IMO including them does no harm, > rather it does some small amount of good, so why not do it? > > You can't really use the fact they were not there before as a reason > to not add them now -- There were no Asserts in the original code > because this same logic was duplicated multiple times and was always > within obvious scope of a particular switch (action) case: >
I see your point but like Hou-San I am also not really sure if these new Asserts will be better. The patch looks good to me, so will push in some time. -- With Regards, Amit Kapila.