Re: Huge memory consumption on partitioned table with FKs

At Thu, 3 Dec 2020 16:41:45 +0900, Amit Langote <amitlangote09@gmail.com> wrote in 
> On Thu, Dec 3, 2020 at 2:29 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > At Thu, 3 Dec 2020 12:27:53 +0900, Amit Langote <amitlangote09@gmail.com> wrote in
> > > On Thu, Dec 3, 2020 at 10:15 AM Kyotaro Horiguchi
> > > <horikyota.ntt@gmail.com> wrote:
> > > For the queries on the referencing side ("check" side),
> > > type/collation/attribute name determined using the above are going to
> > > be the same for all partitions in a given tree irrespective of the
> > > attribute number, because they're logically the same column.  On the
> >
> > Yes, I know that, which is what I meant by "practically" or
> > "actually", but it is not explicitly defined AFAICS.
> 
> Well, I think it's great that we don't have to worry *in this part of
> the code* about partition's fk_attnums not being congruent with the
> root parent's, because ensuring that is the responsibility of the
> other parts of the system such as DDL.  If we have any problems in
> this area, they should be dealt with by ensuring that there are no
> bugs in those other parts.

Agreed.

> > Thus that would be no longer an issue if we explicitly define that
> > "When conpraentid stores a valid value, each element of fk_attnums
> > points to logically the same attribute with the RI_ConstraintInfo for
> > the parent constraint."  Or I'd be happy if we have such a comment
> > there instead.
> 
> I saw a comment in Kuroda-san's v2 patch that is perhaps meant to
> address this point, but the placement needs to be reconsidered:

Ah, yes, that comes from my proposal.

> @@ -366,6 +368,14 @@ RI_FKey_check(TriggerData *trigdata)
>         querysep = "WHERE";
>         for (int i = 0; i < riinfo->nkeys; i++)
>         {
> +
> +           /*
> +           * We share the same plan among all relations in a partition
> +           * hierarchy.  The plan is guaranteed to be compatible since all of
> +           * the member relations are guaranteed to have the equivalent set
> +           * of foreign keys in fk_attnums[].
> +           */
> +
>             Oid         pk_type = RIAttType(pk_rel, riinfo->pk_attnums[i]);
>             Oid         fk_type = RIAttType(fk_rel, riinfo->fk_attnums[i]);
> 
> A more appropriate place for this kind of comment would be where
> fk_attnums is defined or in ri_BuildQueryKey() that is shared by
> different RI query issuing functions.

Yeah, I wanted more appropriate place for the comment.  That place
seems reasonable.

> > > referenced side ("restrict", "cascade", "set" side), as you already
> > > mentioned, fk_attnums refers to the top parent table of the
> > > referencing side, so no possibility of they being different in the
> > > various referenced partitions' RI_ConstraintInfos.
> >
> > Right. (I'm not sure I have mention that here, though:p)A
> 
> Maybe I misread but I think you did in your email dated Dec 1 where you said:
> 
> "After an off-list discussion, we confirmed that even in that case the
> patch works as is because fk_attnum (or contuple.conkey) always stores
> key attnums compatible to the topmost parent when conparent has a
> valid value (assuming the current usage of fk_attnum), but I still
> feel uneasy to rely on that unclear behavior."

fk_attnums *doesn't* refers to to top parent talbe of the referencing
side. it refers to attributes of the partition that is compatible with
the same element of fk_attnums of the topmost parent.  Maybe I'm
misreading.


> > > On the topic of how we'd be able to share even the RI_ConstraintInfos
> > > among partitions, that would indeed look a bit more elaborate than the
> > > patch we have right now.
> >
> > Maybe just letting the hash entry for the child riinfo point to the
> > parent riinfo if all members (other than constraint_id, of course)
> > share the exactly the same values.  No need to count references since
> > we don't going to remove riinfos.
> 
> Ah, something maybe worth trying.  Although the memory we'd save by
> sharing the RI_ConstraintInfos would not add that much to the savings
> we're having by sharing the plan, because it's the plans that are a
> memory hog AFAIK.

I agree that plans are rather large but the sharable part of the
RI_ConstraintInfos is 536 bytes, I'm not sure it is small enough
comparing to the plans.  But that has somewhat large footprint.. (See
the attached)

> > > > About your patch, it calculates the root constrid at the time an
> > > > riinfo is created, but when the root-partition is further attached to
> > > > another partitioned-table after the riinfo creation,
> > > > constraint_root_id gets stale.  Of course that dones't matter
> > > > practically, though.
> > >
> > > Maybe we could also store the hash value of the root constraint OID as
> > > rootHashValue and check for that one too in
> > > InvalidateConstraintCacheCallBack().  That would take care of this
> > > unless I'm missing something.
> >
> > Seems to be sound.
> 
> Okay, thanks.
> 
> I have attached a patch in which I've tried to merge the ideas from
> both my patch and Kuroda-san's.  I liked that his patch added
> conparentid to RI_ConstraintInfo because that saves a needless
> syscache lookup for constraints that don't have a parent.  I've kept
> my idea to compute the root constraint id only once in
> ri_LoadConstraint(), not on every invocation of ri_BuildQueryKey().
> Kuroda-san, anything you'd like to add to that?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center