Re: pg_restore causing deadlocks on partitioned tables

On Wed, Sep 16, 2020 at 2:41 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > On Tue, Sep 15, 2020 at 7:28 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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