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

Hello all,

as this is my first mail to pgsql-hackers, please be gentle :)

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".

node->ss.ps.ps_numTuples is f.i. an uint64.

Or is there a specific reason the limit must be a double?

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.

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

And this comment might be better "were we already called?"

>+ bool rs_started; /* are we already called? */

Hope this helps, and thank you for working on this issue.

Regards,

Tels