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,


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to