Check for integer overflow in datetime functions

On Wed, 2005-11-30 at 19:36 -0700, Michael Fuhr wrote:

Check integer conversion for overflow in datetime functions.

It seems a bit laborious to always manually set errno to zero before
invoking strtol() (both in the places added by the patch, and all the
places that already did that). While it's only a minor notational
improvement, I wonder if it would be worth adding a pg_strtol() that did
the errno assignment so that each call-site wouldn't need to worry about
it?

-Neil

Neil Conway <neilc@samurai.com> writes:

It seems a bit laborious to always manually set errno to zero before
invoking strtol() (both in the places added by the patch, and all the
places that already did that). While it's only a minor notational
improvement, I wonder if it would be worth adding a pg_strtol() that did
the errno assignment so that each call-site wouldn't need to worry about
it?

Don't see the point really, since the check of errno would still have to
be in the calling code (since pg_strtol wouldn't know what the
appropriate error action is). So the choice is between

errno = 0;
foo = strtol(...);
if (errno == ERANGE)
... something ...

and

foo = pg_strtol(...);
if (errno == ERANGE)
... something ...

I don't think there's less cognitive load in the second case,
particularly not to someone who's familiar with the standard behavior
of strtol() --- you'll be constantly thinking "what if errno is already
ERANGE ... oh, pg_strtol resets it ..."

I'd be in favor of encapsulating the whole sequence including an
ereport(ERROR) into one function, except it seems we'd not use it in
very many places.

BTW, Neil, were you looking at this with intention to commit it?
I'll pick it up if not, but don't want to duplicate effort if you've
already started on it.

regards, tom lane

Michael Fuhr <mike@fuhr.org> writes:

Check integer conversion for overflow in datetime functions.

Looks reasonable, will check and apply.

This patch should apply cleanly against HEAD and 8.1. If the patch
looks good then I'll submit patches for earlier branches when I get
a chance to build and test those versions.

Don't worry about that, I'll take care of it. I prefer committing all
the branches at once when doing a multi-branch fix (less clutter in
the CVS logs).

regards, tom lane

Tom Lane wrote:

Don't worry about that, I'll take care of it. I prefer committing all
the branches at once when doing a multi-branch fix (less clutter in
the CVS logs).

How do you do that? I have multiple checked-out trees, I assume you do
the same and just handle the simultaneous-ness by hand?

BTW, has anyone checked Command Prompt's Subversion repository? It's a
mirror of our anonymous CVS (AFAICT). I'm using it for reading diffs
lately, and it's much nicer to look at the whole patch as a single diff
rather than going a single file at a time.

http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql

It has the additional advantage over our current CVSweb that it's set
with tabs to 4 spaces, so it looks just like our code is supposed to ...

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

Alvaro Herrera <alvherre@commandprompt.com> writes:

Tom Lane wrote:

Don't worry about that, I'll take care of it. I prefer committing all
the branches at once when doing a multi-branch fix (less clutter in
the CVS logs).

How do you do that? I have multiple checked-out trees, I assume you do
the same and just handle the simultaneous-ness by hand?

Well, they're not *exactly* simultaneous of course. I set up the
modified files in each tree and then commit, commit, commit (the -F
option helps if the commit message is long). I use cvs2cl to extract
CVS history, and it will fold together commits in different branches
if they have the same commit message and are within some time delta
of each other ... I think it's 5 minutes or so.

regards, tom lane

Michael Fuhr <mike@fuhr.org> writes:

Check integer conversion for overflow in datetime functions.

Applied back to 7.4. The patch would not work in 7.3 because we didn't
have the DTERR_ convention back then, and it seems not worth the effort
to try to deal with the problem that far back.

In one place (line 60 in the patch) the code sets errno to 0 when
it should still be 0 after similar code a few lines above (otherwise
the function would have returned an error). I wasn't sure what
would be preferred: clearing errno again "just to be sure," a comment
explaining why it isn't necessary, or nothing at all.

I left it as-is ...

regards, tom lane

BTW, has anyone checked Command Prompt's Subversion
repository? It's a mirror of our anonymous CVS (AFAICT).
I'm using it for reading diffs lately, and it's much nicer to
look at the whole patch as a single diff rather than going a
single file at a time.

http://projects.commandprompt.com/projects/public/pgsql/browse
r/trunk/pgsql

It has the additional advantage over our current CVSweb that
it's set with tabs to 4 spaces, so it looks just like our
code is supposed to ...

Yes, and I for one love it :-)

Now if I could just access the actual svn repository as well, I'd be
even happier...

//Magnus

BTW, has anyone checked Command Prompt's Subversion repository? It's a
mirror of our anonymous CVS (AFAICT). I'm using it for reading diffs
lately, and it's much nicer to look at the whole patch as a single diff
rather than going a single file at a time.

http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql

It has the additional advantage over our current CVSweb that it's set
with tabs to 4 spaces, so it looks just like our code is supposed to ...

Does anyone else find the PostgreSQL "TRUNK" funny? :D

Magnus Hagander wrote:

BTW, has anyone checked Command Prompt's Subversion
repository? It's a mirror of our anonymous CVS (AFAICT).
I'm using it for reading diffs lately, and it's much nicer to
look at the whole patch as a single diff rather than going a
single file at a time.

http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql

It has the additional advantage over our current CVSweb that
it's set with tabs to 4 spaces, so it looks just like our
code is supposed to ...

Yes, and I for one love it :-)

Now if I could just access the actual svn repository as well, I'd be
even happier...

I have added it to the set of useful links on pgfoundry's front page.

cheers

andrew

http://projects.commandprompt.com/projects/public/pgsql/browser/trunk/pgsql

It has the additional advantage over our current CVSweb that it's
set with tabs to 4 spaces, so it looks just like our code is
supposed to ...

I need to spend some time on it to see if there is a way that I can
convert more then the whole tree at a time within the same repository.
Right now we have to convert
the whole thing which takes quite a bit of time.

Oh and don't forget to use the RSS capabilities.

Yes, and I for one love it :-)

Now if I could just access the actual svn repository as well, I'd be
even happier...

We are also happy to provide accounts to those who would like to
actually use the wiki to discuss specific aspects of the code. This
would be more for wannabe hacker edibility,
tips, tricks etc...

I have added it to the set of useful links on pgfoundry's front page.

Cool!

Joshua D. Drake

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

--
The PostgreSQL Company - Command Prompt, Inc. 1.503.667.4564
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Managed Services, Shared and Dedicated Hosting
Co-Authors: PLphp, PLperl - http://www.commandprompt.com/