On Tue, Jul 25, 2023 at 05:16:56PM -0700, Nathan Bossart wrote: > [v4] If we're not going to have a general SQL conversion function, here are some comments on the current patch.
+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? - *ptr = '\0'; + Assert(base == 2 || base == 8 || base == 16); + *ptr = '\0'; Spurious whitespace change? - 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. +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. +{ oid => '5101', descr => 'convert int4 number to binary', Needs to be over 8000. -- John Naylor EDB: http://www.enterprisedb.com