Re: a misbehavior of partition row movement (?)

On Sun, Mar 20, 2022 at 5:13 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2022-Mar-18, Zhihong Yu wrote:
>
> > +#define AFTER_TRIGGER_OFFSET 0x07FFFFFF /* must be low-order
> > bits */
> > +#define AFTER_TRIGGER_DONE 0x80000000
> > +#define AFTER_TRIGGER_IN_PROGRESS 0x40000000
> >
> > Is it better if the order of AFTER_TRIGGER_DONE
> > and AFTER_TRIGGER_IN_PROGRESS is swapped (for the ordinal values to be
> > sequential) ?
>
> They *are* sequential -- See
> https://www.postgresql.org/message-id/202201172215.2tse3vjjgi2b%40alvherre.pgsql
>
> > +#define AFTER_TRIGGER_CP_UPDATE 0x08000000
> >
> > It would be better to add a comment for this constant, explaining what CP
> > means (cross partition).
>
> Sure.

Thanks.

> > + if (!partRel->rd_rel->relispartition)
> > + elog(ERROR, "cannot find ancestors of a non-partition result
> > relation");
> >
> > It would be better to include the relation name in the error message.
>
> I don't think it matters. We don't really expect to hit this.

I tend to think maybe showing at least the OID in the error message
doesn't hurt, but maybe we don't need to.

> > + /* Ignore the root ancestor, because ...?? */
> >
> > Please fill out the remainder of the comment.
>
> I actually would like to know what's the rationale for this myself.
> Amit?

Ah, it's just that the ancstorRels list contains *all* ancestors,
including the root one, whose triggers will actually be fired to
enforce its foreign key. The code below the line of code that this
comment is for is to check *non-root* ancestor's triggers to spot any
that look like they enforce foreign keys to flag them as
unenforceable.

I've fixed the comment as:

- /* Ignore the root ancestor, because ...?? */
+ /* Root ancestor's triggers will be processed. */

> > + if (!trig->tgisclone &&
> > + RI_FKey_trigger_type(trig->tgfoid) == RI_TRIGGER_PK)
> > + {
> > + has_noncloned_fkey = true;
> >
> > The variable says fkey, but the constant is not RI_TRIGGER_FK. Maybe add a
> > comment explaining why.
>
> Well, the constant is about the trigger *function*, not about any
> constraint. This code is testing "is this a noncloned trigger, and does
> that trigger use an FK-related function?" If you have a favorite
> comment to include, I'm all ears.

A description of what we're looking for with this code is in the
comment above the loop:

/*
* For any foreign keys that point directly into a non-root ancestors of
* the source partition,...

So finding triggers in those non-root ancestors whose function is
RI_TRIGGER_PK tells us that those relations have foreign keys pointing
into it or that it is the PK table in that relationship. Other than
the comment, the code itself seems self-documenting with regard to
what's being done (given the function/macro/variable naming), so maybe
we're better off without additional commentary here.

I've attached a delta patch on v16 for the above comment and a couple
of other changes.

--
Amit Langote
EDB: http://www.enterprisedb.com