Re: [PATCH] Add function to_oct

On Tue, Aug 15, 2023 at 01:53:25PM +0700, John Naylor wrote:
> If we're not going to have a general SQL conversion function, here are some
> comments on the current patch.

I appreciate the review.

> +static char *convert_to_base(uint64 value, int base);
>
> Not needed if the definition is above the callers.

Done.

> + * Workhorse for to_binary, to_oct, and to_hex. Note that base must be
> either
> + * 2, 8, or 16.
>
> Why wouldn't it work with any base <= 16?

You're right. I changed this in v5.

> - *ptr = '\0';
> + Assert(base == 2 || base == 8 || base == 16);
>
> + *ptr = '\0';
>
> Spurious whitespace change?

I think this might just be a weird artifact of the diff algorithm.

> - char buf[32]; /* bigger than needed, but reasonable */
> + char *buf = palloc(sizeof(uint64) * BITS_PER_BYTE + 1);
>
> Why is this no longer allocated on the stack? Maybe needs a comment about
> the size calculation.

It really should be. IIRC I wanted to avoid passing a pre-allocated buffer
to convert_to_base(), but I don't remember why. I fixed this in v5.

> +static char *
> +convert_to_base(uint64 value, int base)
>
> On my machine this now requires a function call and a DIV instruction, even
> though the patch claims not to support anything but a power of two. It's
> tiny enough to declare it inline so the compiler can specialize for each
> call site.

This was on my list of things to check before committing. I assumed that
it would be automatically inlined, but given your analysis, I went ahead
and added the inline keyword. My compiler took the hint and inlined the
function, and it used SHR instead of DIV instructions. The machine code
for to_hex32/64 is still a couple of instructions longer than before
(probably because of the uint64 casts), but I don't know if we need to
worry about micro-optimizing these functions any further.

> +{ oid => '5101', descr => 'convert int4 number to binary',
>
> Needs to be over 8000.

Done.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com