Re: Size of Path nodes

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Dec 5, 2015 at 12:35 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
>>> To add to whatever has been said above, intention of adding that flag
>>> was also to avoid adding new nodes for parallelism. Basically modify
>>> the existing nodes (like SeqScan) to take care of whatever is needed
>>> for parallel execution.

>> TBH, I would say that that's a damn-fool idea. I think you should instead
>> create a separate ParallelSeqScan path type and plan type, and the same
>> for every other thing that becomes parallel-aware.

> Maybe. But if we go down that path, we're eventually going to have
> ParallelSeqScan, ParallelIndexScan, ParallelBitmapHeapScan,
> ParallelForeignScan, ParallelAppend, ParallelHashJoin, and probably a
> bunch of others. That could lead to a lot of code duplication. Even
> for ParallelSeqScan:

> src/backend/executor/nodeSeqscan.c | 136
> ++++++++++++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 103 insertions(+), 33 deletions(-)

> Now that file has 344 lines today, but only 33 of those lines are
> actual changes to pre-existing code, and most of those are mechanical.
> So the effect of making that a whole separate node type would have
> been to copy about 200 lines of code and comments. That didn't seem
> like a good idea.

Meh. Size of first patch is a pretty bad guide to whether a particular
structural decision is going to be a good idea in the long run. Do you
need some examples of patches we've rejected because they were done in a
way that minimized the size of the patch rather than a way that made sense
from a system structural viewpoint?

I would also point out that if we'd followed this approach in the past,
nodeIndexscan.c, nodeBitmapIndexscan.c and nodeIndexonlyscan.c would be
one file, and it would be nigh unintelligible. Those files nonetheless
manage to share code where it makes sense for them to.

In any case, even if there's an argument for keeping the two cases
together in the executor, I remain of the opinion that you'd be better off
with them being distinct Path types in the planner.

regards, tom lane