On Mon, Oct 12, 2020 at 3:42 PM Greg Nancarrow <gregn4...@gmail.com> wrote: > On Sun, Oct 11, 2020 at 1:39 PM Thomas Munro <thomas.mu...@gmail.com> wrote: > > pathnode->path.total_cost += subpath->total_cost; > > - pathnode->path.rows += subpath->rows; > > + if (returningLists != NIL) > > + pathnode->path.rows += subpath->rows; > > total_size += subpath->pathtarget->width * subpath->rows; > > }
Erm, except the condition should of course cover total_size too. > Agree, thanks (bug in existing Postgres code, right?) Yeah, I think we should go ahead and fix that up front. Here's a version with a commit message.
From d92c45e6c1fa5ed118195de149d8fb833ea008ef Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Mon, 12 Oct 2020 15:49:27 +1300 Subject: [PATCH] Fix row estimate for ModifyTable paths. Historically, we estimated that a ModifyTable node would emit the same number of rows as it writes. That's only true if there is a RETURNING clause, otherwise the real number is zero. It didn't matter much before, but proposed patches to allow for parallel writes revealed that such queries would look unduly expensive if we continued to charge for a phantom RETURNING clause. Correct the estimates in master only. There doesn't seem to be much point in back-patching the change for now. Reviewed-by: Greg Nancarrow <gregn4...@gmail.com> Discussion: https://postgr.es/m/CAJcOf-cXnB5cnMKqWEp2E2z7Mvcd04iLVmV%3DqpFJrR3AcrTS3g%40mail.gmail.com --- src/backend/optimizer/util/pathnode.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index c1fc866cbf..d718a4adba 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -3583,16 +3583,13 @@ create_modifytable_path(PlannerInfo *root, RelOptInfo *rel, if (lc == list_head(subpaths)) /* first node? */ pathnode->path.startup_cost = subpath->startup_cost; pathnode->path.total_cost += subpath->total_cost; - pathnode->path.rows += subpath->rows; - total_size += subpath->pathtarget->width * subpath->rows; + if (returningLists != NIL) + { + pathnode->path.rows += subpath->rows; + total_size += subpath->pathtarget->width * subpath->rows; + } } - /* - * Set width to the average width of the subpath outputs. XXX this is - * totally wrong: we should report zero if no RETURNING, else an average - * of the RETURNING tlist widths. But it's what happened historically, - * and improving it is a task for another day. - */ if (pathnode->path.rows > 0) total_size /= pathnode->path.rows; pathnode->path.pathtarget->width = rint(total_size); -- 2.20.1
