> Hello all,
>
> as this is my first mail to pgsql-hackers, please be gentle :)
>
Welcome to pgsql-hackers,
> 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.
> node->ss.ps.ps_numTuples is f.i. an uint64.
>
> Or is there a specific reason the limit must be a double?
>
The above variable represents "actual" number of rows at the execution
stage. Likely, hardware replacement cycle will come before int64 overflow.
> And finally:
>
> >+ if (node->ss.ps.ps_numTuples > 0)
>
> >+ appendStringInfo(&buf, " LIMIT %ld",
> node->ss.ps.ps_numTuples);
>
> vs.
>
> >+ appendStringInfo(&buf, "%s LIMIT %lu",
> >+ sql,
> node->ss.ps.ps_numTuples);
>
> It seems odd to have two different format strings here for the same variable.
>
Ah, yes, %lu is right because ps_numTuples is uint64.
> A few comments miss "." at the end, like these:
>
> >+ * Also, pass down the required number of tuples
>
> >+ * Pass down the number of required tuples by the upper node
>
OK,
> 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;
Thanks,
----
PG-Strom Project / NEC OSS Promotion Center
KaiGai Kohei <kaigai@ak.jp.nec.com>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
