Re: a misbehavior of partition row movement (?)

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, Arne Roland <A(dot)Roland(at)index(dot)de>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Subject: Re: a misbehavior of partition row movement (?)
Date: 2022-01-18 22:29:25
Message-ID: 202201182229.tedjzcsgqsxn@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2022-Jan-18, Alvaro Herrera wrote:

> On 2022-Jan-18, Amit Langote wrote:
>
> > Would you like me to update the patch with the above renumbering or
> > are you working on a new version yourself?
>
> I have a few very minor changes apart from that. Let me see if I can
> get this pushed soon; if I'm unable to (there are parts I don't fully
> grok yet), I'll post what I have.

Here's v13, a series with your patch as 0001 and a few changes from me;
the bulk is just pgindent, and there are a few stylistic changes and an
unrelated typo fix and I added a couple of comments to your new code.

I don't like the API change to ExecInsert(). You're adding two output
arguments:
- the tuple being inserted. But surely you have this already, because
it's in the 'slot' argument you passed in. ExecInsert is even careful to
set the ->tts_tableOid argument there. So ExecInsert's caller doesn't
need to receive the inserted tuple as an argument, it can just read
'slot'.
- the relation to which the tuple was inserted. Can't this be obtained
by looking at slot->tts_tableOid? We should be able to use
ExecLookupResultRelByOid() to obtain it, no? (I suppose you may claim
that this is wasteful, but I think this is not a common case anyway and
it's worth keeping ExecInsert()'s API clean for the sake of the 99.99%
of its other calls).

I think the argument definition of ExecCrossPartitionUpdateForeignKey is
a bit messy. I propose to move mtstate,estate as two first arguments;
IIRC the whole executor does it that way.

AfterTriggerSaveEvent determines maybe_crosspart_update (by looking at
mtstate->operation -- why doesn't it look at 'event' instead?) and later
it determines new_event.ate_flags. Why can't it use
maybe_crosspart_update to simplify part of that? Or maybe the other way
around, not sure. It looks like something could be made simpler there.

Overall, the idea embodied in the patch looks sensible to me.

Thank you,

--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Nunca confiaré en un traidor. Ni siquiera si el traidor lo he creado yo"
(Barón Vladimir Harkonnen)

Attachment Content-Type Size
v13-0001-Enforce-foreign-key-correctly-during-cross-parti.patch text/x-diff 58.0 KB
v13-0002-alvherre-tweaks.patch text/x-diff 20.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-01-18 22:32:44 Re: Add last commit LSN to pg_last_committed_xact()
Previous Message Tom Lane 2022-01-18 22:12:00 Re: pgsql: Modify pg_basebackup to use a new COPY subprotocol for base back