Re: Patch for - Change FETCH/MOVE to use int8

This patch is for the following TODO item.

SQL command:
-/Change LIMIT/OFFSET and FETCH/MOVE to use int8

/Since the limit/offset patch is already applied,
this patch is meant for Fetch/Move query.
I have tested the patch and it works for int64 values.
Please verify this.

Thanks
Dhanaraj
/
/

Dhanaraj M wrote:

I had a quick look:

> ***************
> *** 209,215 ****
>
> /* Return command status if wanted */
> if (completionTag)
> ! snprintf(completionTag, COMPLETION_TAG_BUFSIZE, "%s %ld",
> stmt->ismove ? "MOVE" : "FETCH",
> nprocessed);
> }
> --- 209,215 ----
>
> /* Return command status if wanted */
> if (completionTag)
> ! snprintf(completionTag, COMPLETION_TAG_BUFSIZE, "%s %lld",
> stmt->ismove ? "MOVE" : "FETCH",
> nprocessed);
> }

You shouldn't be using %lld as it breaks on some platforms.
Use INT64_FORMAT instead.

> --- ./src/backend/parser/gram.y Sun Aug 13 00:06:28 2006
> ***************
> *** 116,122 ****
>
> %union
> {
> ! int ival;
> char chr;
> char *str;
> const char *keyword;
> --- 116,122 ----
>
> %union
> {
> ! int64 ival;
> char chr;
> char *str;
> const char *keyword;

I don't think this is the right approach. Maybe it would be reasonable
to add another arm to the %union instead, not sure. The problem is the
amount of ugly casts you have to use below. The scanner code seems to
think that a constant larger than the biggest int4 should be treated as
float, so I'm not sure why this would work anyway.

> ***************
> *** 767,773 ****
> /*
> * Force the queryDesc destination to the right thing. This supports
> * MOVE, for example, which will pass in dest = DestNone. This is okay to
> ! * change as long as we do it on every fetch. (The Executor must not
> * assume that dest never changes.)
> */
> if (queryDesc)
> --- 767,773 ----
> /*
> * Force the queryDesc destination to the right thing. This supports
> * MOVE, for example, which will pass in dest = DestNone. This is okay to
> ! * change as int64 as we do it on every fetch. (The Executor must not
> * assume that dest never changes.)
> */
> if (queryDesc)

Too enthusiastic about the search'n replace I think.

I stopped reading at this point.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> I don't think this is the right approach. Maybe it would be reasonable
> to add another arm to the %union instead, not sure. The problem is the
> amount of ugly casts you have to use below. The scanner code seems to
> think that a constant larger than the biggest int4 should be treated as
> float, so I'm not sure why this would work anyway.

I'm not sure that I see the point of this at all. ISTM the entire
reason for using a cursor is that you're going to fetch the results
in bite-size pieces. I don't see the current Postgres source code
surviving into the era where >2G rows is considered bite-size ;-)

I thought the int8-LIMIT patch was equally pointless, btw, but at
least it was not very invasive. This one is not passing the minimum
usefulness-to-ugliness ratio for me.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > I don't think this is the right approach. Maybe it would be reasonable
> > to add another arm to the %union instead, not sure. The problem is the
> > amount of ugly casts you have to use below. The scanner code seems to
> > think that a constant larger than the biggest int4 should be treated as
> > float, so I'm not sure why this would work anyway.
>
> I'm not sure that I see the point of this at all. ISTM the entire
> reason for using a cursor is that you're going to fetch the results
> in bite-size pieces. I don't see the current Postgres source code
> surviving into the era where >2G rows is considered bite-size ;-)

Think MOVE to a specific section of the cursor > 2gig. I can see that
happening.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Tom Lane wrote:
>> I'm not sure that I see the point of this at all. ISTM the entire
>> reason for using a cursor is that you're going to fetch the results
>> in bite-size pieces. I don't see the current Postgres source code
>> surviving into the era where >2G rows is considered bite-size ;-)

> Think MOVE to a specific section of the cursor > 2gig. I can see that
> happening.

Yeah, and by the time it happens you'll have gotten bored and found
something else to do. With no support in the system for random access
to a cursor result, this is just about as useless as the FETCH case.

In any case I agree with Alvaro's comment: the way to support int8 in
a FETCH/MOVE command is not to try to convert the entire rest of the
grammar to int8 instead of int4 as its native datatype.

regards, tom lane

Hi Alvaro

Thanks for your valuable suggestions.
I made the changes as suggested earlier.
Please review again and comment on this.
I like to make changes if it is required.

Patch applied. Thanks.

I had to convert a lot of whitespace to tabs. It wasn't just the
whitespace, but whitespace that was 8 spaces. I assume you are reading
our code using an 8-space tab. Please see the developer's FAQ and try
to use tabs in future patches. Thanks.

---------------------------------------------------------------------------

Dhanaraj M wrote:
> Hi Alvaro
>
> Thanks for your valuable suggestions.
> I made the changes as suggested earlier.
> Please review again and comment on this.
> I like to make changes if it is required.

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +