Re: a misbehavior of partition row movement (?)

Sawada-san,

On Wed, Mar 10, 2021 at 4:51 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> I looked at the 0001 patch and here are random comments. Please ignore
> a comment if it is already discussed.

Thanks a lot for the review and sorry for the delay in replying.

> ---
> @@ -9077,7 +9102,8 @@ addFkRecurseReferenced(List **wqueue, Constraint
> *fkconstraint, Relation rel,
> partIndexId, constrOid, numfks,
> mapped_pkattnum, fkattnum,
> pfeqoperators, ppeqoperators, ffeqoperators,
> - old_check_ok);
> + old_check_ok,
> + deleteTriggerOid, updateTriggerOid);
>
> /* Done -- clean up (but keep the lock) */
> table_close(partRel, NoLock);
> @@ -9126,8 +9152,12 @@ addFkRecurseReferencing(List **wqueue,
> Constraint *fkconstraint, Relation rel,
> Relation pkrel, Oid indexOid, Oid parentConstr,
> int numfks, int16 *pkattnum, int16 *fkattnum,
> Oid *pfeqoperators, Oid *ppeqoperators, Oid
> *ffeqoperators,
> - bool old_check_ok, LOCKMODE lockmode)
> + bool old_check_ok, LOCKMODE lockmode,
> + Oid parentInsTrigger, Oid parentUpdTrigger)
> {
>
> We need to update the function comments as well.

Done.

> ---
> I think it's better to add comments for newly added functions such as
> GetForeignKeyActionTriggers() and GetForeignKeyCheckTriggers() etc.
> Those functions have no comment at all.

I've added comments.

> BTW, those two functions out of newly added four functions:
> AttachForeignKeyCheckTriggers() and DetachForeignKeyCheckTriggers(),
> have only one user. Can we past the functions body at where each
> function is called?

I made those pieces of code into functions because I thought future
patches may have a need for them. But maybe those future patches
should do the refactoring, so I've incorporated their code into the
respective callers as you suggest.

> ---
> /*
> * If the referenced table is a plain relation, create the action triggers
> * that enforce the constraint.
> */
> - if (pkrel->rd_rel->relkind == RELKIND_RELATION)
> - {
> - createForeignKeyActionTriggers(rel, RelationGetRelid(pkrel),
> - fkconstraint,
> - constrOid, indexOid);
> - }
> + createForeignKeyActionTriggers(rel, RelationGetRelid(pkrel),
> + fkconstraint,
> + constrOid, indexOid,
> + parentDelTrigger, parentUpdTrigger,
> + &deleteTriggerOid, &updateTriggerOid);
>
> The comment needs to be updated.
> ---
> /*
> * If the referencing relation is a plain table, add the check triggers to
> * it and, if necessary, schedule it to be checked in Phase 3.
> *
> * If the relation is partitioned, drill down to do it to its partitions.
> */
> + createForeignKeyCheckTriggers(RelationGetRelid(rel),
> + RelationGetRelid(pkrel),
> + fkconstraint,
> + parentConstr,
> + indexOid,
> + parentInsTrigger, parentUpdTrigger,
> + &insertTriggerOid, &updateTriggerOid);
>
> Same as above.

Done and done.

> ---
> I think TriggerSetParentTrigger needs to free the heap tuple copied by
> heap_copytuple().

Oops, done.

> ---
> + Assert(trigForm->tgparentid == 0);
> + if (trigForm->tgparentid != InvalidOid)
> + elog(ERROR, "trigger %u already has a parent trigger",
> + childTrigId);
>
> I think the first condition in Assert() can be
> !OidIsValid(trigForm->tgparentid) and the second condition in 'if'
> statement can be OidIsValid(trigForm->tgparentid). So those are
> redundant?

Ah, indeed. I've kept the if () elog(...) after applying your suggested change.

> ---
> - if (fout->remoteVersion >= 90000)
> + if (fout->remoteVersion >= 130000)
> + {
> + /*
> + * NB: think not to use pretty=true in pg_get_triggerdef. It
> + * could result in non-forward-compatible dumps of WHEN clauses
> + * due to under-parenthesization.
> + */
> + appendPQExpBuffer(query,
> + "SELECT tgname, "
> + "tgfoid::pg_catalog.regproc AS tgfname, "
> + "pg_catalog.pg_get_triggerdef(oid,
> false) AS tgdef, "
> + "tgenabled, tableoid, oid "
> + "FROM pg_catalog.pg_trigger t "
> + "WHERE tgrelid = '%u'::pg_catalog.oid "
> + "AND NOT tgisinternal "
> + "AND tgparentid = 0",
> + tbinfo->dobj.catId.oid);
> + }
> + else if (fout->remoteVersion >= 90000)
>
> You meant 140000 instead of 130000?

I think I used 130000 because tgparentid was added in v13, but maybe
140000 is right, because for v13 the existing condition (NOT
tgisnternal) already gets us the intended triggers.

> Or is this change really needed? This change added one condition
> "tgparentid = 0" but IIUC I think triggers that are NOT tgisinternal
> are always tgparentid = 0. Also, it seems it is true both before and
> after this patch.

Actually, as noted in the commit message, I'm intending to change
tgisnternal to only be true for triggers generated by foreign keys and
no longer for partitions' user-defined triggers that are inherited.
So whereas NOT tgisnternal would suffice to exclude partitions'
inherited triggers before, that would no longer be the case with this
patch; AND tgparentid = 0 will be needed for that.

> ---
> @@ -2973,11 +2973,7 @@ describeOneTableDetails(const char *schemaname,
> " AND u.tgparentid = 0) AS parent" :
> "NULL AS parent"),
> oid);
> - if (pset.sversion >= 110000)
> - appendPQExpBufferStr(&buf, "(NOT t.tgisinternal OR
> (t.tgisinternal AND t.tgenabled = 'D') \n"
> - " OR EXISTS (SELECT 1 FROM
> pg_catalog.pg_depend WHERE objid = t.oid \n"
> - " AND refclassid =
> 'pg_catalog.pg_trigger'::pg_catalog.regclass))");
> - else if (pset.sversion >= 90000)
> + if (pset.sversion >= 90000)
>
> I think we cannot remove this code. For instance, in PG13 since the
> trisinternal of a user-defined trigger that has descended from its
> parent table is true, executing \d against PG13 by the patched psql
> won't show that trigger.

I think you're right. I simply needed to change the if condition as follows:

- if (pset.sversion >= 110000)
+ if (pset.sversion >= 110000 && pset.sversion < 140000)

Note that without this change, this code ends up revealing partitions'
foreign key triggers, because we will now be marking them dependent on
their parent trigger, which wasn't the case before this patch.

> I'll look at 0002 patch.

Actually, I found a big hole in my assumptions around deferrable
foreign constraints, invalidating the approach I took in 0002 to use a
query-lifetime tuplestore to record root parent tuples. I'm trying to
find a way to make the tuplestore transaction-lifetime so that the
patch still works.

In the meantime, I'm attaching an updated set with 0001 changed per
your comments.

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