Re: a misbehavior of partition row movement (?)

On Fri, Feb 26, 2021 at 4:30 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> Hi Rahila,
>
> On Wed, Feb 24, 2021 at 3:07 PM Rahila Syed <rahilasyed90(at)gmail(dot)com> wrote:
> >> > I think the documentation update is missing from the patches.
> >>
> >> Hmm, I don't think we document the behavior that is improved by the v3
> >> patches as a limitation of any existing feature, neither of foreign
> >> keys referencing partitioned tables nor of the update row movement
> >> feature. So maybe there's nothing in the existing documentation that
> >> is to be updated.
> >>
> >> However, the patch does add a new error message for a case that the
> >> patch doesn't handle, so maybe we could document that as a limitation.
> >> Not sure if in the Notes section of the UPDATE reference page which
> >> has some notes on row movement or somewhere else. Do you have
> >> suggestions?
> >>
> > You are right, I could not find any direct explanation of the impact of row movement during
> > UPDATE on a referencing table in the PostgreSQL docs.
> >
> > The two documents that come close are either:
>
> Thanks for looking those up.
>
> > 1. https://www.postgresql.org/docs/13/trigger-definition.html .
> > The para starting with "If an UPDATE on a partitioned table causes a row to move to another partition"
> > However, this does not describe the behaviour of internal triggers which is the focus of this patch.
>
> The paragraph does talk about a very related topic, but, like you, I
> am not very excited about adding a line here about what we're doing
> with internal triggers.
>
> > 2. Another one like you mentioned, https://www.postgresql.org/docs/11/sql-update.html
> > This has explanation for row movement behaviour for partitioned table but does not explain
> > any impact of such behaviour on a referencing table.
> > I think it is worth adding some explanation in this document. Thus, explaining
> > impact on referencing tables here, as it already describes behaviour of
> > UPDATE on a partitioned table.
>
> ISTM the description of the case that will now be prevented seems too
> obscure to make into a documentation line, but I tried. Please check.
>

I looked at the 0001 patch and here are random comments. Please ignore
a comment if it is already discussed.

---
@@ -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.

---
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.

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?

---
/*
* 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.

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

---
+ 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?

---
- 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?

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.

---
@@ -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'll look at 0002 patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/