Re: PassDownLimitBound for ForeignScan/CustomScan [take-2]

> Hello,
>
> On Wed, March 1, 2017 7:21 pm, Kouhei Kaigai wrote:
> >> I've looked at the patch, and as I'm not that familiar with the
> >> pg-sourcecode, customs and so on, this isn't a review, more like food
> >> for thought and all should be taken with a grain of salt. :)
> >>
> >> So here are a few questions and remarks:
> >>
> >> >+ double limit_tuples = -1.0;
> >>
> >> Surely the limit cannot be fractional, and must be an integer. So
> >> wouldn't it be better the same type as say:
> >>
> >> >+ if (root->limit_tuples >= 0.0 &&
> >>
> >> Than you could also compare with ">= 0", not ">= 0.0".
> >>
> > The above variable represents the "estimated" number of rows at the
> > planning stage, not execution time.
> > You may be able to see Path structure has "rows" field declared as
> > double type. It makes sense to consider stupid paths during planning,
> > even if it is eventually rejected. For example, if a cross join with
> > two large tables appear during planning, 64bit integer will make
> > overflow easily.
>
> Hm, ok. Not related to your patch, just curious: Is there a mechanism in
> place that automatically rejects plans where the limit would overflow the
> double to uint64 conversation? Or is this more of a "there would be hopefully
> a plan with a better limit so we do not use the bad one"?
>
> Would it possible to force a plan where such overflow would occur?
>
We have no such mechanism, and less necessity.
Estimated number of rows in plan time is stored in the plan_rows field of
the Plan structure, as FP64. Once plan-tree gets constructed, estimated
number of rows shall not affect to the execution. (Some plan might use it
for estimation of resource consumption on execution time.)
On the other hands, the actual number of rows that was processed is saved
on the instrument field of the PlanState structure. It is counted up from
the zero by one. So, people wants to replace the hardware prior to uint64
overflow. If 1B rows are processed per sec, uint64 overflow happen at 26th
century(!).

> >> And this comment might be better "were we already called?"
> >>
> >> >+ bool rs_started; /* are we already
> >> called? */
> >>
> > Other variables in ResultState uses present form, like:
> >
> > + bool rs_started; /* are we already called? */
> > bool rs_done; /* are we done? */
> > bool rs_checkqual; /* do we need to check the qual? */
> > } ResultState;
>
> Yes, I noted that, but still "are" and "called" and "already" don't read
> well together for me:
>
> are - present form
> called - past form like "were we called?", or "are we called bob?" an
> ongoing process
> already - it has started
>
> So "are we already called" reads like someone is waiting for being called.
>
> Maybe to mirror the comment on "rs_done":
>
> /* have we started yet? */
>
> Also, maybe it's easier for the comment to describe what is happening in
> the code because of the flag, not just to the flag itself:
>
> /* To do things once when we are called */
>
> Anyway, it is a minor point and don't let me distract you from your work,
> I do like the feature and the patch :)
>
Fixed to "have we started yet?"

----
PG-Strom Project / NEC OSS Promotion Center
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>