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);


>+                      appendStringInfo(&buf, "%s LIMIT %lu",
>+                                                       sql, 

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.



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

Reply via email to