[PATCH] Add crc32(text) & crc32(bytea)

On Thu, Jul 18, 2024 at 02:24:23PM +0300, Aleksander Alekseev wrote:

I vaguely recall that I faced this problem before. Supporting crc32
was requested on the mailing list [2] and a number of workarounds
exist in PL/pgSQL [3][4]. Since there seems to be a demand and it
costs us nothing to maintain crc32() I suggest adding it.

This sounds generally reasonable to me, especially given the apparent
demand. Should we also introduce crc32c() while we're at it?

--
nathan

Hi,

This sounds generally reasonable to me, especially given the apparent
demand. Should we also introduce crc32c() while we're at it?

Might be a good idea. However I didn't see a demand for crc32c() SQL
function yet. Also I'm not sure whether the best interface for it
would be crc32c() or crc32(x, version='c') or perhaps crc32(x,
polinomial=...). I propose keeping the scope small this time.

--
Best regards,
Aleksander Alekseev

On Fri, Jul 26, 2024 at 12:01:40PM +0300, Aleksander Alekseev wrote:

This sounds generally reasonable to me, especially given the apparent
demand. Should we also introduce crc32c() while we're at it?

Might be a good idea. However I didn't see a demand for crc32c() SQL
function yet. Also I'm not sure whether the best interface for it
would be crc32c() or crc32(x, version='c') or perhaps crc32(x,
polinomial=...). I propose keeping the scope small this time.

I don't think adding crc32c() would sufficiently increase the scope. We'd
use the existing implementations for both crc32() and crc32c(). And
besides, this could be useful for adding tests for that code.

+ <function>crc32</function> ( <type>text</type> )

Do we need a version of the function that takes a text input? It's easy
enough to cast to a bytea.

+ <returnvalue>text</returnvalue>

My first reaction is that we should just have this return bytea like the
SHA ones do, if for no other reason than commit 10cfce3 seems intended to
move us away from returning text for these kinds of functions. Upthread,
you mentioned the possibility of returning a bigint, too. I think I'd
still prefer bytea in case we want to add, say, crc64() or crc16() in the
future. That would allow us to keep all of these functions consistent
instead of returning different types for each. However, I understand that
returning the numeric types might be more convenient. I'm curious what
others think about this.

+        Computes the CRC32 <link linkend="functions-hash-note">hash</link> of
+        the binary string, with the result written in hexadecimal.

I'm not sure we should call the check values "hashes." Wikipedia does
include them in the "List of hash functions" page [0]https://en.wikipedia.org/wiki/List_of_hash_functions, but it seems to
deliberately avoid calling them hashes in the CRC page [1]https://en.wikipedia.org/wiki/Cyclic_redundancy_check. I'd suggest
calling them "CRC32 values" instead.

[0]: https://en.wikipedia.org/wiki/List_of_hash_functions
[1]: https://en.wikipedia.org/wiki/Cyclic_redundancy_check

--
nathan

Hi Nathan,

I don't think adding crc32c() would sufficiently increase the scope. We'd
use the existing implementations for both crc32() and crc32c(). And
besides, this could be useful for adding tests for that code.

+ <function>crc32</function> ( <type>text</type> )

Do we need a version of the function that takes a text input? It's easy
enough to cast to a bytea.

+ <returnvalue>text</returnvalue>

My first reaction is that we should just have this return bytea like the
SHA ones do, if for no other reason than commit 10cfce3 seems intended to
move us away from returning text for these kinds of functions. Upthread,
you mentioned the possibility of returning a bigint, too. I think I'd
still prefer bytea in case we want to add, say, crc64() or crc16() in the
future. That would allow us to keep all of these functions consistent
instead of returning different types for each. However, I understand that
returning the numeric types might be more convenient. I'm curious what
others think about this.

+        Computes the CRC32 <link linkend="functions-hash-note">hash</link> of
+        the binary string, with the result written in hexadecimal.

I'm not sure we should call the check values "hashes." Wikipedia does
include them in the "List of hash functions" page [0], but it seems to
deliberately avoid calling them hashes in the CRC page [1]. I'd suggest
calling them "CRC32 values" instead.

Thanks for the code review. Here is the updated patch.

--
Best regards,
Aleksander Alekseev

+/*
+ * Calculate CRC32 of the given data.
+ */
+static inline pg_crc32
+crc32_sz(const char *buf, int size)
+{
+	pg_crc32	crc;
+	const char *p = buf;
+
+	INIT_TRADITIONAL_CRC32(crc);
+	while (size > 0)
+	{
+		char		c = (char) (*p);
+
+		COMP_TRADITIONAL_CRC32(crc, &c, 1);
+		size--;
+		p++;
+	}
+	FIN_TRADITIONAL_CRC32(crc);
+	return crc;
+}

I'm curious why we need to do this instead of only using the macros:

INIT_TRADITIONAL_CRC32(crc);
COMP_TRADITIONAL_CRC32(crc, VARDATA_ANY(in), len);
FIN_TRADITIONAL_CRC32(crc);

+ * IDENTIFICATION
+ * src/backend/utils/adt/hashfuncs.c

Perhaps these would fit into src/backend/utils/hash/pg_crc.c?

--
nathan

Hi,

I'm curious why we need to do this instead of only using the macros:

INIT_TRADITIONAL_CRC32(crc);
COMP_TRADITIONAL_CRC32(crc, VARDATA_ANY(in), len);
FIN_TRADITIONAL_CRC32(crc);

+ * IDENTIFICATION
+ * src/backend/utils/adt/hashfuncs.c

Perhaps these would fit into src/backend/utils/hash/pg_crc.c?

Thanks, PFA patch v3.

--
Best regards,
Aleksander Alekseev

On Mon, Aug 05, 2024 at 04:19:45PM +0300, Aleksander Alekseev wrote:

Thanks, PFA patch v3.

This looks pretty good to me. The only point that I think deserves more
discussion is the return type. Does bytea make the most sense here? Or
should we consider int/bigint?

--
nathan

Hi,

This looks pretty good to me. The only point that I think deserves more
discussion is the return type. Does bytea make the most sense here? Or
should we consider int/bigint?

Personally I would choose BYTEA in order to be consistent with sha*() functions.

It can be casted to TEXT if user wants a result similar to the one
md5() returns:

```
SELECT encode(crc32('PostgreSQL'), 'hex');
```

... and somewhat less convenient to BIGINT:

```
SELECT ((get_byte(crc, 0) :: bigint << 24) | (get_byte(crc, 1) << 16)
| (get_byte(crc, 2) << 8) | get_byte(crc, 3))
FROM (SELECT crc32('PostgreSQL') AS crc);
```

I don't like the `integer` option because crc32 value is typically
considered as an unsigned one and `integer` is not large enough to
represent uint32.

Perhaps we need get_int4() / get_int8() / get_numeric() as there seems
to be a demand [1]https://stackoverflow.com/questions/32944267/postgresql-converting-bytea-to-bigint[2]/messages/by-id/AANLkTikip9xs8iXc8e+Mgz1T1701i8Xk6QtbVB3KJQzX@mail.gmail.com and it will allow us to easily cast a `bytea`
value to `integer` or `bigint`. This is probably another topic though.

[1]: https://stackoverflow.com/questions/32944267/postgresql-converting-bytea-to-bigint
[2]: /messages/by-id/AANLkTikip9xs8iXc8e+Mgz1T1701i8Xk6QtbVB3KJQzX@mail.gmail.com

--
Best regards,
Aleksander Alekseev

On Tue, Aug 06, 2024 at 11:04:41AM +0300, Aleksander Alekseev wrote:

Perhaps we need get_int4() / get_int8() / get_numeric() as there seems
to be a demand [1][2] and it will allow us to easily cast a `bytea`
value to `integer` or `bigint`. This is probably another topic though.

Yeah, I was surprised to learn there wasn't yet an easy way to do this.
I'm not sure how much of a factor this should play in deciding the return
value for the CRC functions, but IMHO it's a reason to reconsider returning
text as you originally proposed.

--
nathan

Hi,

Yeah, I was surprised to learn there wasn't yet an easy way to do this.
I'm not sure how much of a factor this should play in deciding the return
value for the CRC functions, but IMHO it's a reason to reconsider returning
text as you originally proposed.

OK, here is the corrected patch v4.

--
Best regards,
Aleksander Alekseev

On 05.08.24 17:28, Nathan Bossart wrote:

This looks pretty good to me. The only point that I think deserves more
discussion is the return type. Does bytea make the most sense here? Or
should we consider int/bigint?

The correct return type of a CRC operation in general is some kind of
exact numerical type. Just pick the best one that fits the result. I
don't think bytea is appropriate.

On Thu, Aug 08, 2024 at 04:27:20PM +0200, Peter Eisentraut wrote:

On 05.08.24 17:28, Nathan Bossart wrote:

This looks pretty good to me. The only point that I think deserves more
discussion is the return type. Does bytea make the most sense here? Or
should we consider int/bigint?

The correct return type of a CRC operation in general is some kind of exact
numerical type. Just pick the best one that fits the result. I don't think
bytea is appropriate.

That would leave us either "integer" or "bigint". "integer" is more
correct from a size perspective, but will result in negative values because
it is signed. "bigint" uses twice as many bytes but won't display any CRC
values as negative.

I guess we could also choose "numeric", which would set a more sustainable
precedent if we added functions for CRC-64...

--
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Aug 08, 2024 at 04:27:20PM +0200, Peter Eisentraut wrote:

The correct return type of a CRC operation in general is some kind of exact
numerical type. Just pick the best one that fits the result. I don't think
bytea is appropriate.

That would leave us either "integer" or "bigint". "integer" is more
correct from a size perspective, but will result in negative values because
it is signed. "bigint" uses twice as many bytes but won't display any CRC
values as negative.

bigint seems fine to me; we have used that in other places as a
substitute for uint32, eg block numbers in contrib/pageinspect.

regards, tom lane

On Thu, Aug 08, 2024 at 10:49:42AM -0400, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:

On Thu, Aug 08, 2024 at 04:27:20PM +0200, Peter Eisentraut wrote:

The correct return type of a CRC operation in general is some kind of exact
numerical type. Just pick the best one that fits the result. I don't think
bytea is appropriate.

That would leave us either "integer" or "bigint". "integer" is more
correct from a size perspective, but will result in negative values because
it is signed. "bigint" uses twice as many bytes but won't display any CRC
values as negative.

bigint seems fine to me; we have used that in other places as a
substitute for uint32, eg block numbers in contrib/pageinspect.

WFM. Here is what I have staged for commit.

--
nathan

Hi,

WFM. Here is what I have staged for commit.

Patch v5 LGTM.

--
Best regards,
Aleksander Alekseev

On Mon, Aug 12, 2024 at 04:13:02PM +0300, Aleksander Alekseev wrote:

Patch v5 LGTM.

Committed.

--
nathan