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?

>> 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 :)

Best wishes,

Tels