Em sex., 28 de ago. de 2020 às 04:45, <gkokola...@pm.me> escreveu: > > > > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ > On Friday, 28 August 2020 03:22, Ranier Vilela <ranier...@gmail.com> > wrote: > > > Hi, > > > > Per Coverity. > > > > When "Prepare for toasting", it is necessary to turn off the flag > TOAST_NEEDS_DELETE_OLD, > > if there is no need to delete external values from the old tuple, > otherwise, > > there are dereference NULL at toast_helper.c (on toast_tuple_cleanup > function). > > > > Excuse my ignorance, isn't this a false positive? > Yes, you're right.
Coverity fails with &.
if (oldtup == NULL)
147 {
3. assign_zero: Assigning: ttc.ttc_oldvalues = NULL.
148 ttc.ttc_oldvalues = NULL;
149 ttc.ttc_oldisnull = NULL;
4. Falling through to end of if statement.
150 }
151 else
152 {
153 ttc.ttc_oldvalues = toast_oldvalues;
154 ttc.ttc_oldisnull = toast_oldisnull;
155 }
156 ttc.ttc_attr = toast_attr;
157 toast_tuple_init(&ttc); // Coverity ignores the call completely
here.
toast_tuple_init, solves the bug, because reset ttc->flags.
> Regardless right after prepare for toasting, a call to toast_tuple_init is
> made which will explicitly and unconditionally set ttc_flags to zero so the
> flag bit set in the patch will be erased anyways. This patch may make
> coverity happy but does not really change anything in the behaviour of the
> code.
>
That's right, the patch doesn't change anything.
>
> Furthermore, in the same function, toast_tuple_init, the flag is set to
> TOAST_NEEDS_DELETE_OLD after the old value is actually inspected and found
> to not be null, be stored on disk and to be different than the new value.
> To my understanding, this seems to be correct.
>
Very correct.
Thanks for taking a look here.
You could take a look at the attached patch,
would it be an improvement?
toast_tuple_init, it seems to me that it can be improved.
ttc->ttc_oldvalues is constant, and it could be unlooping?
regards,
Ranier Vilela
unloop_toast_tuple_init.patch
Description: Binary data
