On Wed, Sep 16, 2020 at 2:41 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > 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.
Yeah, the Assert seems pretty pointless at this point. > 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. Ah, you're right. > 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.) Agreed that it's not really necessary to optimize that case. Updated patch attached. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
remove-ri_PartitionCheck_v2.patch
Description: Binary data