Re: more unconstify use

On 13/02/2019 19:51, Mark Dilger wrote:
> Peter, so sorry I did not review this patch before you committed.  There
> are a few places where you unconstify the argument to a function where
> changing the function to take const seems better to me.  I argued for
> something similar in 2016,

One can consider unconstify a "todo" marker.  Some of these could be
removed by some API changes.  It needs case-by-case consideration.

> Your change:
> 
> -            md5_calc((uint8 *) (input + i), ctxt);
> +            md5_calc(unconstify(uint8 *, (input + i)), ctxt);
> 
> Perhaps md5_calc's signature should change to
> 
>     md5_calc(const uint8 *, md5_ctxt *)
> 
> since it doesn't modify the first argument.

Fixed, thanks.

> Your change:
> 
> -        if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) newtup, newsz))
> +        if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) unconstify(BrinTuple *, newtup), newsz))
> 
> Perhaps PageIndexTupleOverwrite's third argument should be const, since it
> only uses it as the const source of a memcpy.  (This is a bit harder than
> for md5_calc, above, since the third argument here is of type Item, which
> is itself a typedef to Pointer, and there exists no analogous ConstPointer
> or ConstItem definition in the sources.)

Yeah, typedefs to a pointer are a poor fit for this.  We have been
getting rid of them from time to time, but I don't know a general solution.

> Your change:
> 
> -            XLogRegisterBufData(0, (char *) newtup, newsz);
> +            XLogRegisterBufData(0, (char *) unconstify(BrinTuple *, newtup), newsz);
> 
> Perhaps the third argument to XLogRegisterBufData can be changed to const,
> with the extra work of changing XLogRecData's data field to const.  I'm not
> sure about the amount of code churn this would entail.

IIRC, the XLogRegister stuff is a web of lies with respect to constness.
 Resolving this properly is tricky.

> Your change:
> 
> -        result = pg_be_scram_exchange(scram_opaq, input, inputlen,
> +        result = pg_be_scram_exchange(scram_opaq, unconstify(char *, input), inputlen,
>                                        &output, &outputlen,
>                                        logdetail);
> 
> pg_be_scram_exchange passes the second argument into two functions,
> read_client_first_message and read_client_final_message, both of which take
> it as a non-const argument but immediately pstrdup it such that it might
> as well have been const.  Perhaps we should just clean up this mess rather
> than unconstifying.

Also fixed!

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services