Subject: Re: a misbehavior of partition row movement (?)

From: Zhihong Yu <[email-masked]>
Date: Mon, 17 Jan 2022 07:20:22 -0800

On Mon, Jan 17, 2022 at 6:26 AM Alvaro Herrera <[email-masked]>
wrote:

> On 2022-Jan-17, Amit Langote wrote:
>
> > Note that the fix involves adding fields to ResultRelInfo -- v13 needs
> > 2 additional, while v14 and HEAD need 1.  That combined with needing
> > new catalog entries for parent FK triggers, back-patching this does
> > make me a bit uncomfortable.
>
> Yeah, that's a good point, so I ran abidiff on the binaries in branch 13
> to have some data on it.  The report does indeed have a lot of noise
> about those three added members in struct ResultRelInfo; but as far as I
> can see in the report, there is no ABI affected because of these
> changes.
>
> However, the ones that caught my eye next were the ABI changes for
> ExecGetTriggerResultRel() and ExecAR{Delete,Update}Triggers().  These seem
> more
> significant, if any external code is calling these.  Now, while I think
> we could dodge that (at least part of it) by having a shim for
> AfterTriggerSaveEvent that passes a NULL mtstate, and takes the
> assumption that there is no row partition migration when that happens
> ... that seems like treading in dangerous territory: we would have
> code that would behave differently for an extension that was compiled
> with an earlier copy of the backend.
>
> So I see two options.  One is to introduce the aforementioned shim, but
> instead of making any assumptions, we cause the shim raise an error:
> "your extension is outdated, please recompile with the new postgres
> version".  However, that seems even more harmful, because production
> systems that auto-update to the next Postgres version would start to
> fail.
>
> The other is suggested by you:
>
> > Another thing to consider is that we haven't seen many reports of the
> > problem (UPDATEs of partitioned PK tables causing DELETEs in
> > referencing tables), even though it can be possibly very surprising to
> > those who do run into it.
>
> Do nothing in the old branches.
>
>
> Another thing I saw which surprised me very much is this bit, which I
> think must be a bug in abidiff:
>
>                                 type of 'EPQState* EState::es_epq_active'
> changed:
>                                   in pointed to type 'struct EPQState' at
> execnodes.h:1104:1:
>                                     type size hasn't changed
>                                     1 data member changes (3 filtered):
>                                       type of 'PlanState*
> EPQState::recheckplanstate' changed:
>                                         in pointed to type 'struct
> PlanState' at execnodes.h:1056:1:
>                                           entity changed from 'struct
> PlanState' to compatible type 'typedef PlanState' at execnodes.h:1056:1
>
> Hi,
I think option 2, not backpatching, is more desirable at this stage.

If, complaints from the field arise in the future, we can consider
backpatching.

Cheers

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 17 Jan 2022 13:29:20 -0300

> I think option 2, not backpatching, is more desirable at this stage.

Preliminarly, I tend to agree.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/