Amit Langote <amitlangot...@gmail.com> writes:
> On Tue, Sep 15, 2020 at 7:28 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>> AFAICS, it is utterly silly for InitResultRelInfo to be forcing
>> a partition qual to be computed when we might not need it.
>> We could flush ResultRelInfo.ri_PartitionCheck altogether and
>> have anything that was reading it instead do
>> RelationGetPartitionQual(ResultRelInfo.ri_RelationDesc).
> Yeah, makes sense. Please see attached a patch to do that.
Just eyeballing this, this bit seems bogus:
@@ -1904,7 +1903,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
Bitmapset *insertedCols;
Bitmapset *updatedCols;
- Assert(constr || resultRelInfo->ri_PartitionCheck);
+ Assert(constr);
if (constr && constr->has_not_null)
{
It does look like all the call sites check for the rel having constraints
before calling, so the modified Assert may not be failing ... but why
are we asserting and then also making a run-time test?
My inclination is to just drop the Assert as useless. There's no
particular reason for this function to make it a hard requirement
that callers optimize away unnecessary calls.
I'm suspicious of the business in ExecPartitionCheck about constructing
a constant-true expression. I think executing that is likely to add
more cycles than you save by not running through this code each time;
once relcache has cached the knowledge that the partition expression
is empty, all the steps here are pretty darn cheap ... which no doubt
is why there wasn't a comparable optimization already. If you're
really concerned about that it'd be better to add a separate
"bool ri_PartitionCheckExprValid" flag. (Perhaps that's worth doing
to avoid impacts from relcache flushes; though I remain unconvinced
that optimizing for the empty-expression case is useful.)
regards, tom lane
