Re: [PATCH] Add function to_oct

From: Nathan Bossart <nathandbossart(at)gmail(dot)com>
To: John Naylor <john(dot)naylor(at)enterprisedb(dot)com>
Cc: Kirk Wolak <wolakk(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Eric Radman <ericshane(at)eradman(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [PATCH] Add function to_oct
Date: 2023-08-15 17:17:15
Message-ID: 20230815171715.GA2312399@nathanxps13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.


> + * 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.


Nathan Bossart
Amazon Web Services:

Attachment Content-Type Size
v5-0001-add-to_binary-and-to_oct.patch text/x-diff 8.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2023-08-15 20:31:09 Re: run pgindent on a regular basis / scripted manner
Previous Message Robert Haas 2023-08-15 17:08:36 Re: initial pruning in parallel append