Re: [PATCH] Add get_bytes() and set_bytes() functions

Hi Dean,

> This should use ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE, rather than
> ERRCODE_INVALID_PARAMETER_VALUE, for consistency with other similar
> errors.
>
> The bytea -> int[248] cast functions should not be marked as leakproof
> -- see the docs on the CREATE FUNCTION page: functions that raise
> errors for some input values but not others, are not leakproof. This
> is why, for example, the int -> bigint cast is leakproof, but the
> bigint -> int cast is not.
>
> Functions working with int8 values should normally go in
> utils/adt/int8.c, not utils/adt/int.c. However, I think that
> utils/adt/varlena.c would be a better place for all these functions,
> because they have more to do with bytea than integer types, and this
> would allow them to be kept together, similar to how all the bit <->
> integer cast functions are in utils/adt/varbit.c.
>
> There's no documentation for these new casts. The obvious place to put
> it would be in section 9.5 "Binary String Functions and Operators",
> which would be consistent with the idea that these are being regarded
> primarily as bytea operations, rather than integer operations (just as
> the bit <-> integer casts are documented in 9.6 "Bit String Functions
> and Operators").

Many thanks for your great feedback. Here is the corrected patch.

On Fri, Jan 17, 2025 at 7:29 PM Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
wrote:

> On Tue, 14 Jan 2025 at 13:25, Aleksander Alekseev
> <aleksander(at)timescale(dot)com> wrote:
> >
> > Thanks. I agree that the proposed error messages look nicer than the
> > one I used in v6. Here is the corrected patch.
> >
>
> This should use ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE, rather than
> ERRCODE_INVALID_PARAMETER_VALUE, for consistency with other similar
> errors.
>
> The bytea -> int[248] cast functions should not be marked as leakproof
> -- see the docs on the CREATE FUNCTION page: functions that raise
> errors for some input values but not others, are not leakproof. This
> is why, for example, the int -> bigint cast is leakproof, but the
> bigint -> int cast is not.
>
> Functions working with int8 values should normally go in
> utils/adt/int8.c, not utils/adt/int.c. However, I think that
> utils/adt/varlena.c would be a better place for all these functions,
> because they have more to do with bytea than integer types, and this
> would allow them to be kept together, similar to how all the bit <->
> integer cast functions are in utils/adt/varbit.c.
>
> There's no documentation for these new casts. The obvious place to put
> it would be in section 9.5 "Binary String Functions and Operators",
> which would be consistent with the idea that these are being regarded
> primarily as bytea operations, rather than integer operations (just as
> the bit <-> integer casts are documented in 9.6 "Bit String Functions
> and Operators").
>
> Regards,
> Dean
>

--
Best regards,
Aleksander Alekseev