On Fri, Jan 28, 2022 at 2:19 PM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2022-01-28 17:27:37 -0300, Alvaro Herrera wrote: > MERGE, v10. I am much more comfortable with this version; I have > removed a bunch of temporary hacks and cleaned up the interactions with > table AM and executor, which is something that had been bothering me for > a while. The complete set of changes can be seen in github, > https://github.com/alvherre/postgres/commits/merge-15
Any chance you could split this into something more reviewable? The overall diff stats are: 102 files changed, 8589 insertions(+), 234 deletions(-) thats pretty hard to really review. Incremental commits don't realy help with that.
> I am not aware of anything of significance in terms of remaining work > for this project.
One thing from skimming: There's not enough documentation about the approach imo - it's a complicated enough feature to deserve more than the one paragraph in src/backend/executor/README.
I'm still [1],[2] uncomfortable with execMerge.c kinda-but-not-really being an executor node.
> The one thing I'm a bit bothered about is the fact that we expose a lot of > executor functions previously static. I am now wondering if it would be > better to move the MERGE executor support functions into nodeModifyTable.c, > which I think would mean we would not have to expose those function > prototypes.
I'm worried about the complexity of nodeModifyTuple.c as is. Moving more code
Hi,
I think you meant nodeModifyTable.c.
And I agree with your comment (on not making nodeModifyTable.c bigger).
Cheers
in there does not seem like the right call to me - we should do the opposite if anything.
A few inline comments below. No real review, just stuff noticed while skimming to see where this is at.