On 2017/01/21 9:01, Tom Lane wrote: > Robert Haas <robertmh...@gmail.com> writes: >> The difference is that those other equalBLAH functions call a >> carefully limited amount of code whereas, in looking over the >> backtrace you sent, I realized that equalPartitionDescs is calling >> partition_bounds_equal which does this: >> cmpval = >> DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[j], >> key->partcollation[j], >> b1->datums[i][j], >> b2->datums[i][j])) > > Ah, gotcha. > >> That's of course opening up a much bigger can of worms. But apart >> from the fact that it's unsafe, I think it's also wrong, as I said >> upthread. I think calling datumIsEqual() there should be better all >> around. Do you think that's unsafe here? > > That sounds like a plausible solution. It is safe in the sense of > being a bounded amount of code. It would return "false" in various > interesting cases like toast pointer versus detoasted equivalent, > but I think that would be fine in this application.
Sorry for jumping in late. Attached patch replaces the call to partitioning-specific comparison function by the call to datumIsEqual(). I wonder if it is safe to assume that datumIsEqual() would return true for a datum and copy of it made using datumCopy(). The latter is used to copy a single datum from a bound's Const node (what is stored in the catalog for every bound). > It would probably be a good idea to add something to datumIsEqual's > comment to the effect that trying to make it smarter would be a bad idea, > because some callers rely on it being stupid. I assume "making datumIsEqual() smarter" here means to make it account for toasting of varlena datums, which is not a good idea because some of its callers may be working in the context of an aborted transaction. I tried to update the header comment along these lines, though please feel to rewrite it. Thanks, Amit
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index ad95b1bc55..8c9a373f1f 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -639,12 +639,14 @@ partition_bounds_equal(PartitionKey key, continue; } - /* Compare the actual values */ - cmpval = DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[j], - key->partcollation[j], - b1->datums[i][j], - b2->datums[i][j])); - if (cmpval != 0) + /* + * Compare the actual values; note that we do not invoke the + * partitioning specific comparison operator here, because we + * could be in an aborted transaction. + */ + if (!datumIsEqual(b1->datums[i][j], b2->datums[i][j], + key->parttypbyval[j], + key->parttyplen[j])) return false; } diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c index 535e4277cc..071a7d4db1 100644 --- a/src/backend/utils/adt/datum.c +++ b/src/backend/utils/adt/datum.c @@ -209,6 +209,10 @@ datumTransfer(Datum value, bool typByVal, int typLen) * of say the representation of zero in one's complement arithmetic). * Also, it will probably not give the answer you want if either * datum has been "toasted". + * + * Do not try to make this any smarter than it currently is with respect + * to "toasted" datums, because some of the callers could be working in the + * context of an aborted transaction. *------------------------------------------------------------------------- */ bool
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
