Re: [PATCH] Add function to_oct

On Thu, 22 Dec 2022 at 23:11, Eric Radman <ericshane(at)eradman(dot)com> wrote:
>
> On Thu, Dec 22, 2022 at 10:08:17AM +0000, Dag Lem wrote:
> >
> > The calculation of quotient and remainder can be replaced by less costly masking and shifting.
> >
> > Defining
> >
> > #define OCT_DIGIT_BITS 3
> > #define OCT_DIGIT_BITMASK 0x7
> >
> > the content of the loop can be replaced by
> >
> > *--ptr = digits[value & OCT_DIGIT_BITMASK];
> > value >>= OCT_DIGIT_BITS;
> >
> > Also, the check for ptr > buf in the while loop can be removed. The
> > check is superfluous, since buf cannot possibly be exhausted by a 32
> > bit integer (the maximum octal number being 37777777777).
>
> I integrated these suggestions in the attached -v2 patch and tested
> range of values manually.
>
> Also picked an OID > 8000 as suggested by unused_oids.

Few suggestions
1) We could use to_oct instead of to_oct32 as we don't have multiple
implementations for to_oct
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 98d90d9338..fde0b24563 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -3687,6 +3687,9 @@
{ oid => '2090', descr => 'convert int8 number to hex',
proname => 'to_hex', prorettype => 'text', proargtypes => 'int8',
prosrc => 'to_hex64' },
+{ oid => '8335', descr => 'convert int4 number to oct',
+ proname => 'to_oct', prorettype => 'text', proargtypes => 'int4',
+ prosrc => 'to_oct32' },

2) Similarly we could change this to "to_oct"
+/*
+ * Convert an int32 to a string containing a base 8 (oct) representation of
+ * the number.
+ */
+Datum
+to_oct32(PG_FUNCTION_ARGS)
+{
+ uint32 value = (uint32) PG_GETARG_INT32(0);

3) I'm not sure if AS "77777777" is required, but I also noticed it
is done similarly in to_hex too:
+--
+-- test to_oct
+--
+select to_oct(256*256*256 - 1) AS "77777777";
+ 77777777
+----------
+ 77777777
+(1 row)

4) You could add a commit message for the patch

Regards,
Vignesh