Re: [HACKERS] 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