Date: Mon, 17 Jan 2022 11:25:53 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	Ibrar Ahmed <ibrar.ahmad@gmail.com>,
	Arne Roland <A.Roland@index.de>,
	"David G. Johnston" <david.g.johnston@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Rahila Syed <rahilasyed90@gmail.com>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>
Subject: Re: a misbehavior of partition row movement (?)
Message-ID: <202201171425.wmli5geb4qi3@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqH2gWBDUotwtnY7qnp+K5j0J4tzUDycqcePA7Yy1s4MbQ@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202201171425.wmli5geb4qi3%40alvherre.pgsql>
Precedence: bulk

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


-- 
Álvaro Herrera           39°49'30"S 73°17'W  —  https://www.EnterpriseDB.com/
"La grandeza es una experiencia transitoria.  Nunca es consistente.
Depende en gran parte de la imaginación humana creadora de mitos"
(Irulan)