[PATCH] Add function to_oct

Hello!

This patch is a new function based on the implementation of to_hex(int).

Since support for octal integer literals was added, to_oct(int) allows
octal values to be easily stored and returned in query results.

to_oct(0o755) = '755'

This is probably most useful for storing file system permissions.

--
Eric Radman

2022年12月21日(水) 7:08 Eric Radman <ericshane(at)eradman(dot)com>:>
> Hello!
>
> This patch is a new function based on the implementation of to_hex(int).
>
> Since support for octal integer literals was added, to_oct(int) allows
> octal values to be easily stored and returned in query results.
>
> to_oct(0o755) = '755'
>
> This is probably most useful for storing file system permissions.

Seems like it would be convenient to have. Any reason why there's
no matching "to_oct(bigint)" version?

Patch has been added to the next commitfest [1], thanks.

[1] https://commitfest.postgresql.org/41/4071/

Regards

Ian Barwick

On Wed, Dec 21, 2022 at 08:36:40AM +0900, Ian Lawrence Barwick wrote:
> 2022年12月21日(水) 7:08 Eric Radman <ericshane(at)eradman(dot)com>:>
> > Hello!
> >
> > This patch is a new function based on the implementation of to_hex(int).
> >
> > Since support for octal integer literals was added, to_oct(int) allows
> > octal values to be easily stored and returned in query results.
> >
> > to_oct(0o755) = '755'
> >
> > This is probably most useful for storing file system permissions.
>
> Seems like it would be convenient to have. Any reason why there's
> no matching "to_oct(bigint)" version?

I couldn't think of a reason someone might want an octal
representation of a bigint. Certainly it would be easy to add
if there is value in supporting all of the same argument types.

2022年12月21日(水) 10:42 Eric Radman <ericshane(at)eradman(dot)com>:
>
> On Wed, Dec 21, 2022 at 08:36:40AM +0900, Ian Lawrence Barwick wrote:
> > 2022年12月21日(水) 7:08 Eric Radman <ericshane(at)eradman(dot)com>:>
> > > Hello!
> > >
> > > This patch is a new function based on the implementation of to_hex(int).
> > >
> > > Since support for octal integer literals was added, to_oct(int) allows
> > > octal values to be easily stored and returned in query results.
> > >
> > > to_oct(0o755) = '755'
> > >
> > > This is probably most useful for storing file system permissions.
> >
> > Seems like it would be convenient to have. Any reason why there's
> > no matching "to_oct(bigint)" version?
>
> I couldn't think of a reason someone might want an octal
> representation of a bigint. Certainly it would be easy to add
> if there is value in supporting all of the same argument types.

Yeah, I am struggling to think of a practical application other than
symmetry with to_hex().

Regards

Ian Barwick

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

This is a mini-review of to_oct :-)

The function seems useful to me, and is obviously correct.

I don't know whether optimization at such a low level is considered in PostgreSQL, but here goes.

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).

Best regards

Dag Lem

The new status of this patch is: Waiting on Author

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.

..Eric

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

vignesh C <vignesh21(at)gmail(dot)com> writes:
> Few suggestions
> 1) We could use to_oct instead of to_oct32 as we don't have multiple
> implementations for to_oct

That seems (a) shortsighted and (b) inconsistent with the naming
pattern used for to_hex, so I doubt it'd be an improvement.

regards, tom lane

On 20.12.22 23:08, Eric Radman wrote:
> This patch is a new function based on the implementation of to_hex(int).
>
> Since support for octal integer literals was added, to_oct(int) allows
> octal values to be easily stored and returned in query results.
>
> to_oct(0o755) = '755'
>
> This is probably most useful for storing file system permissions.

Note this subsequent discussion about the to_hex function:
https://www.postgresql.org/message-id/flat/CAEZATCVbkL1ynqpsKiTDpch34%3DSCr5nnau%3DnfNmiy2nM3SJHtw%40mail.gmail.com

Also, I think there is no "to binary" function, so perhaps if we're
going down this road one way or the other, we should probably complete
the set.

On Thu, Feb 23, 2023 at 6:32 AM Peter Eisentraut <
peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:

> On 20.12.22 23:08, Eric Radman wrote:
> > This patch is a new function based on the implementation of to_hex(int).
> >
> > Since support for octal integer literals was added, to_oct(int) allows
> > octal values to be easily stored and returned in query results.
> >
> > to_oct(0o755) = '755'
> >
> > This is probably most useful for storing file system permissions.
>
> Note this subsequent discussion about the to_hex function:
>
> https://www.postgresql.org/message-id/flat/CAEZATCVbkL1ynqpsKiTDpch34%3DSCr5nnau%3DnfNmiy2nM3SJHtw%40mail.gmail.com
>
> Also, I think there is no "to binary" function, so perhaps if we're
> going down this road one way or the other, we should probably complete
> the set.
>
> The code reads clearly. It works as expected (being an old PDP-11
guy!)... And the docs make sense and build as well.
Nothing larger than an int gets in. I was "missing" the bigint version,
but read through ALL of the messages to see (and agree)
That that's okay.
Marked Ready for Committer.

Thanks, Kirk