On Mon, Jan 25, 2021 at 10:42:25PM -0500, Sehrope Sarkuni wrote:
> +1 to adding a SHA1 SQL function. Even if it's deprecated, there's plenty
> of historical usage that I can see it being useful.
Let's wait for more opinions to see if we agree that this addition is
helpful or not. Even if this is not added, I think that there is
still value in refactoring the code anyway for the SHA-2 functions.
> Either way, the rest of the refactor can be improved a bit to perform a
> single palloc() and remove the memcpy(). Attached is a diff for
> cryptohashfuncs.c that does that by writing the digest final directly to
> the result. It also removes the digest length arg and determines it in the
> switch block. There's only one correct digest length for each type so
> there's no reason to give callers the option to give the wrong one.
Yeah, what you have here is better.
+ default:
+ elog(ERROR, "unsupported digest type %d", type);
Not using a default clause is the purpose here, as it would generate a
compilation warning if a value in the enum is forgotten. Hence, if a
new option is added to pg_cryptohash_type in the future, people won't
miss that they could add a SQL function for the new option. If we
decide that MD5 and SHA1 have no need to use this code path, I'd
rather just use elog(ERROR) instead.
--
Michael
