From 54a94064407375ac3c108e1b791303a42baf030a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 16 Nov 2020 16:39:59 -0500 Subject: [PATCH] Do not return NULL for error cases in satisfies_hash_partition(). Since this function is used as a CHECK constraint condition, returning NULL is tantamount to returning TRUE, which would have the effect of letting in a row that doesn't satisfy the hash condition. Admittedly, the cases for which this is done should be unreachable in practice, but that doesn't make it any less a bad idea. It also seems like a dartboard was used to decide which error cases should throw errors as opposed to returning NULL. For the checks for NULL input values, I just switched it to returning false. There's some argument that an error would be better; but the case really should be can't-happen in a generated hash constraint, so it's likely not worth more code for. For the parent-relation-open-failure case, it seems like we might as well let relation_open throw an error, instead of having an impossible-to-diagnose constraint failure. Back-patch to v11 where this code came in. Discussion: https://postgr.es/m/24067.1605134819@sss.pgh.pa.us --- src/backend/partitioning/partbounds.c | 15 +++++++-------- src/test/regress/expected/hash_part.out | 10 +++------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c index 1db87b92b9..cebd1c1dca 100644 --- a/src/backend/partitioning/partbounds.c +++ b/src/backend/partitioning/partbounds.c @@ -2778,6 +2778,8 @@ compute_partition_hash_value(int partnatts, FmgrInfo *partsupfunc, Oid *partcoll * * Returns true if remainder produced when this computed single hash value is * divided by the given modulus is equal to given remainder, otherwise false. + * NB: it's important that this never return null, as the constraint machinery + * would consider that to be a "pass". * * See get_qual_for_hash() for usage. */ @@ -2802,9 +2804,9 @@ satisfies_hash_partition(PG_FUNCTION_ARGS) ColumnsHashData *my_extra; uint64 rowHash = 0; - /* Return null if the parent OID, modulus, or remainder is NULL. */ + /* Return false if the parent OID, modulus, or remainder is NULL. */ if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2)) - PG_RETURN_NULL(); + PG_RETURN_BOOL(false); parentId = PG_GETARG_OID(0); modulus = PG_GETARG_INT32(1); remainder = PG_GETARG_INT32(2); @@ -2833,15 +2835,12 @@ satisfies_hash_partition(PG_FUNCTION_ARGS) PartitionKey key; int j; - /* Open parent relation and fetch partition keyinfo */ - parent = try_relation_open(parentId, AccessShareLock); - if (parent == NULL) - PG_RETURN_NULL(); + /* Open parent relation and fetch partition key info */ + parent = relation_open(parentId, AccessShareLock); key = RelationGetPartitionKey(parent); /* Reject parent table that is not hash-partitioned. */ - if (parent->rd_rel->relkind != RELKIND_PARTITIONED_TABLE || - key->strategy != PARTITION_STRATEGY_HASH) + if (key == NULL || key->strategy != PARTITION_STRATEGY_HASH) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("\"%s\" is not a hash partitioned table", diff --git a/src/test/regress/expected/hash_part.out b/src/test/regress/expected/hash_part.out index 91ec7c6f58..4c74e4b706 100644 --- a/src/test/regress/expected/hash_part.out +++ b/src/test/regress/expected/hash_part.out @@ -10,11 +10,7 @@ CREATE TABLE mchash1 PARTITION OF mchash FOR VALUES WITH (MODULUS 4, REMAINDER 0); -- invalid OID, no such table SELECT satisfies_hash_partition(0, 4, 0, NULL); - satisfies_hash_partition --------------------------- - -(1 row) - +ERROR: could not open relation with OID 0 -- not partitioned SELECT satisfies_hash_partition('tenk1'::regclass, 4, 0, NULL); ERROR: "tenk1" is not a hash partitioned table @@ -34,14 +30,14 @@ ERROR: remainder for hash partition must be less than modulus SELECT satisfies_hash_partition('mchash'::regclass, NULL, 0, NULL); satisfies_hash_partition -------------------------- - + f (1 row) -- remainder is null SELECT satisfies_hash_partition('mchash'::regclass, 4, NULL, NULL); satisfies_hash_partition -------------------------- - + f (1 row) -- too many arguments