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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
