On Thu, Mar 30, 2023 at 11:12 AM Peter Smith <smithpb2250@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.
