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)
