Thread: Insert result does not match record count
Hi all, I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario: insert into newtable select data from oldtable where proc_date >= x and proc_date < y; INSERT 0 78551642 select count(*) from newtable where proc_date >= x and proc_date < y; count ----------- 4373518938 select count(*) from oldtable where proc_date >= x and proc_date < y; count ----------- 4373518938 So, my counts from the old and new tables match, but the result returned from the insert statement is sometimes a completelydifferent number. (But not always.) I've checked my date ranges very carefully to make sure they match. Has anyone else ever seen this before? Should I be concerned? Thanks, Natalie
Natalie Wenz <nataliewenz@ebureau.com> writes:
> I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario:
> ...
> So, my counts from the old and new tables match, but the result returned from the insert statement is sometimes a
completelydifferent number. (But not always.) I've checked my date ranges very carefully to make sure they match.
Perhaps you've got triggers or rules on the target table that are
suppressing some inserts?
regards, tom lane
No triggers, no rules. It's just a very boring, vanilla table. I have had plenty of cases where the inserts fail because many of the data types are different in the new table, and there is some junk that fails the cast. And even though the insert result seems to indicate that it only inserted some of the rows, when I do a count of the records after the insert, I find the same number of records in the new as in the old table. So it appears to be working correctly, but it's unsettling to see the insert result not match. Do I trust the count, or the insert result? Could this be a sign of some kind of corruption? Not sure how worried I should be.
Cheers,
Natalie
On Jul 19, 2013, at 6:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Natalie Wenz <nataliewenz@ebureau.com> writes:I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario:
...
So, my counts from the old and new tables match, but the result returned from the insert statement is sometimes a completely different number. (But not always.) I've checked my date ranges very carefully to make sure they match.
Perhaps you've got triggers or rules on the target table that are
suppressing some inserts?
regards, tom lane
--
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general
So, in an attempt to see if it was a fluke, I picked one of the date ranges I was getting a different count for, and deleted the records, and then ran the insert again. Interestingly, the delete result matched the insert result (which was the same the second time as it was the first). I'll run a count on the records again, but it will take several hours. I'll post that information when I have it!
On Jul 22, 2013, at 10:44 AM, Natalie Wenz <nataliewenz@ebureau.com> wrote:
No triggers, no rules. It's just a very boring, vanilla table. I have had plenty of cases where the inserts fail because many of the data types are different in the new table, and there is some junk that fails the cast. And even though the insert result seems to indicate that it only inserted some of the rows, when I do a count of the records after the insert, I find the same number of records in the new as in the old table. So it appears to be working correctly, but it's unsettling to see the insert result not match. Do I trust the count, or the insert result? Could this be a sign of some kind of corruption? Not sure how worried I should be.Cheers,NatalieOn Jul 19, 2013, at 6:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:Natalie Wenz <nataliewenz@ebureau.com> writes:I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario:
...
So, my counts from the old and new tables match, but the result returned from the insert statement is sometimes a completely different number. (But not always.) I've checked my date ranges very carefully to make sure they match.
Perhaps you've got triggers or rules on the target table that are
suppressing some inserts?
regards, tom lane
--
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general
On Fri, Jul 19, 2013 at 3:20 PM, Natalie Wenz <nataliewenz@ebureau.com> wrote: > Hi all, > > I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario: > > insert into newtable select data from oldtable where proc_date >= x and proc_date < y; > > INSERT 0 78551642 > > select count(*) from newtable where proc_date >= x and proc_date < y; > count > ----------- > 4373518938 It looks to me like the status report is 32 bits and overflowed. 4,373,518,938 - 2^32 = 78,551,642 Cheers, Jeff
I had just started to wonder if it was something that when you responded. I checked, and every case that I had where thenumbers were wonky checked out when I subtracted 2^32. Whew! Thanks, Jeff! On Jul 22, 2013, at 11:20 AM, Jeff Janes <jeff.janes@gmail.com> wrote: > On Fri, Jul 19, 2013 at 3:20 PM, Natalie Wenz <nataliewenz@ebureau.com> wrote: >> Hi all, >> >> I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario: >> >> insert into newtable select data from oldtable where proc_date >= x and proc_date < y; >> >> INSERT 0 78551642 >> >> select count(*) from newtable where proc_date >= x and proc_date < y; >> count >> ----------- >> 4373518938 > > It looks to me like the status report is 32 bits and overflowed. > > 4,373,518,938 - 2^32 = 78,551,642 > > Cheers, > > Jeff > > > -- > Sent via pgsql-general mailing list (pgsql-general@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-general
On 07/22/2013 06:20 PM, Jeff Janes wrote: > On Fri, Jul 19, 2013 at 3:20 PM, Natalie Wenz <nataliewenz@ebureau.com> wrote: >> Hi all, >> >> I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario: >> >> insert into newtable select data from oldtable where proc_date >= x and proc_date < y; >> >> INSERT 0 78551642 >> >> select count(*) from newtable where proc_date >= x and proc_date < y; >> count >> ----------- >> 4373518938 > It looks to me like the status report is 32 bits and overflowed. > > 4,373,518,938 - 2^32 = 78,551,642 Attached is a small patch that should fix the problem. Vik
Attachment
On 07/24/2013 04:04 PM, Vik Fearing wrote: > On 07/22/2013 06:20 PM, Jeff Janes wrote: >> On Fri, Jul 19, 2013 at 3:20 PM, Natalie Wenz <nataliewenz@ebureau.com> wrote: >>> Hi all, >>> >>> I am moving some data from one table to another in 9.2.4, and keep seeing this strange scenario: >>> >>> insert into newtable select data from oldtable where proc_date >= x and proc_date < y; >>> >>> INSERT 0 78551642 >>> >>> select count(*) from newtable where proc_date >= x and proc_date < y; >>> count >>> ----------- >>> 4373518938 >> It looks to me like the status report is 32 bits and overflowed. >> >> 4,373,518,938 - 2^32 = 78,551,642 > Attached is a small patch that should fix the problem. It would seem this was not as thorough as it should have been and there's a lot more to do. I'm working on a better patch. Also worth mentioning is bug #7766. http://www.postgresql.org/message-id/E1Tlli5-0007tR-HO@wrigleys.postgresql.org Vik
Vik Fearing <vik.fearing@dalibo.com> writes: > Also worth mentioning is bug #7766. > http://www.postgresql.org/message-id/E1Tlli5-0007tR-HO@wrigleys.postgresql.org Yeah, did you read that whole thread? The real issue here is going to be whether client-side code falls over on wider-than-32-bit counts. We can fix the backend and be pretty sure that we've found all the relevant places inside it, but we'll just be exporting the issue. I did look at libpq and noted that it doesn't seem to have any internal problem, because it returns the count to callers as a string (!). But what do you think are the odds that callers are using code that won't overflow? I'd bet on finding atoi() or suchlike in a lot of callers. Even if they thought to use strtoul(), unsigned long is not necessarily 64 bits wide. regards, tom lane
On 2013-07-24 13:48:23 -0400, Tom Lane wrote: > Vik Fearing <vik.fearing@dalibo.com> writes: > > Also worth mentioning is bug #7766. > > http://www.postgresql.org/message-id/E1Tlli5-0007tR-HO@wrigleys.postgresql.org > > Yeah, did you read that whole thread? The real issue here is going to > be whether client-side code falls over on wider-than-32-bit counts. > We can fix the backend and be pretty sure that we've found all the > relevant places inside it, but we'll just be exporting the issue. > I did look at libpq and noted that it doesn't seem to have any internal > problem, because it returns the count to callers as a string (!). > But what do you think are the odds that callers are using code that > won't overflow? I'd bet on finding atoi() or suchlike in a lot of > callers. Even if they thought to use strtoul(), unsigned long is > not necessarily 64 bits wide. Application code that relies on the values already has problems though since the returned values are pretty bogus now. Including the fact that it can return 0 as the number of modified rows which is checked for more frequently than the actual number IME... So I think client code that uses simplistic stuff like atoi isn't worse off afterwards since the values will be about as bogus. I am more worried about code that does range checks like java's string conversion routines... I think fixing this for 9.4 is fine, but due to the compat issues I think it's to late for 9.3. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> I think fixing this for 9.4 is fine, but due to the compat issues I
> think it's to late for 9.3.
Agreed -- this is effectively a protocol change, albeit a pretty minor
one, so I can't see back-patching it.
regards, tom lane
On Wed, Jul 24, 2013 at 08:08:32PM +0200, Andres Freund wrote: > On 2013-07-24 13:48:23 -0400, Tom Lane wrote: > > Vik Fearing <vik.fearing@dalibo.com> writes: > > > Also worth mentioning is bug #7766. > > > http://www.postgresql.org/message-id/E1Tlli5-0007tR-HO@wrigleys.postgresql.org > > > > Yeah, did you read that whole thread? The real issue here is going to > > be whether client-side code falls over on wider-than-32-bit counts. > > We can fix the backend and be pretty sure that we've found all the > > relevant places inside it, but we'll just be exporting the issue. > > > I did look at libpq and noted that it doesn't seem to have any internal > > problem, because it returns the count to callers as a string (!). > > But what do you think are the odds that callers are using code that > > won't overflow? I'd bet on finding atoi() or suchlike in a lot of > > callers. Even if they thought to use strtoul(), unsigned long is > > not necessarily 64 bits wide. > > Application code that relies on the values already has problems though > since the returned values are pretty bogus now. Including the fact that > it can return 0 as the number of modified rows which is checked for more > frequently than the actual number IME... > So I think client code that uses simplistic stuff like atoi isn't worse > off afterwards since the values will be about as bogus. I am more > worried about code that does range checks like java's string conversion > routines... > > I think fixing this for 9.4 is fine, but due to the compat issues I > think it's to late for 9.3. Where are we on this? There was a posted patch, attached, but Vik Fearing said it was insufficent and he was working on a new one: http://www.postgresql.org/message-id/51EFF67A.7020509@dalibo.com -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
On 01/31/2014 06:19 PM, Bruce Momjian wrote: > On Wed, Jul 24, 2013 at 08:08:32PM +0200, Andres Freund wrote: >> On 2013-07-24 13:48:23 -0400, Tom Lane wrote: >>> Vik Fearing <vik.fearing@dalibo.com> writes: >>>> Also worth mentioning is bug #7766. >>>> http://www.postgresql.org/message-id/E1Tlli5-0007tR-HO@wrigleys.postgresql.org >>> Yeah, did you read that whole thread? The real issue here is going to >>> be whether client-side code falls over on wider-than-32-bit counts. >>> We can fix the backend and be pretty sure that we've found all the >>> relevant places inside it, but we'll just be exporting the issue. >>> I did look at libpq and noted that it doesn't seem to have any internal >>> problem, because it returns the count to callers as a string (!). >>> But what do you think are the odds that callers are using code that >>> won't overflow? I'd bet on finding atoi() or suchlike in a lot of >>> callers. Even if they thought to use strtoul(), unsigned long is >>> not necessarily 64 bits wide. >> Application code that relies on the values already has problems though >> since the returned values are pretty bogus now. Including the fact that >> it can return 0 as the number of modified rows which is checked for more >> frequently than the actual number IME... >> So I think client code that uses simplistic stuff like atoi isn't worse >> off afterwards since the values will be about as bogus. I am more >> worried about code that does range checks like java's string conversion >> routines... >> >> I think fixing this for 9.4 is fine, but due to the compat issues I >> think it's to late for 9.3. > Where are we on this? There was a posted patch, attached, but Vik > Fearing said it was insufficent and he was working on a new one: > > http://www.postgresql.org/message-id/51EFF67A.7020509@dalibo.com > Unfortunately, I gave up on it as being over my head when I noticed I was changing the protocol itself. I should have notified the list so someone else could have taken over. -- Vik
On Fri, Jan 31, 2014 at 06:34:27PM +0100, Vik Fearing wrote: > >> Application code that relies on the values already has problems though > >> since the returned values are pretty bogus now. Including the fact that > >> it can return 0 as the number of modified rows which is checked for more > >> frequently than the actual number IME... > >> So I think client code that uses simplistic stuff like atoi isn't worse > >> off afterwards since the values will be about as bogus. I am more > >> worried about code that does range checks like java's string conversion > >> routines... > >> > >> I think fixing this for 9.4 is fine, but due to the compat issues I > >> think it's to late for 9.3. > > Where are we on this? There was a posted patch, attached, but Vik > > Fearing said it was insufficent and he was working on a new one: > > > > http://www.postgresql.org/message-id/51EFF67A.7020509@dalibo.com > > > > Unfortunately, I gave up on it as being over my head when I noticed I > was changing the protocol itself. I should have notified the list so > someone else could have taken over. OK, so that brings up a good question. Can we change the protocol for this without causing major breakage? Tom seems to indicate that it can be done for 9.4, but I thought protocol breakage was a major issue. Are we really changing the wire protocol here, or just the type of string we can pass back to the interface? I know the libpq API we give to clients is a string so it is OK. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Jan 31, 2014 at 06:34:27PM +0100, Vik Fearing wrote:
>> Unfortunately, I gave up on it as being over my head when I noticed I
>> was changing the protocol itself. I should have notified the list so
>> someone else could have taken over.
> OK, so that brings up a good question. Can we change the protocol for
> this without causing major breakage? Tom seems to indicate that it can
> be done for 9.4, but I thought protocol breakage was a major issue. Are
> we really changing the wire protocol here, or just the type of string we
> can pass back to the interface?
What I said about it upthread was "this is effectively a protocol change,
albeit a pretty minor one, so I can't see back-patching it".
The discussion in bug #7766 shows that some client-side code is likely to
need fixing, and that such fixing might actually be nontrivial for them.
So changing this in a minor release is clearly a bad idea. But I don't
have a problem with widening the counters in a major release where we
can document it as a potential compatibility issue.
I took a quick look and noted that CMDSTATUS_LEN and
COMPLETION_TAG_BUFSIZE are set to 64, and have been for quite a long time,
so command status string buffer sizes should not be a problem.
I think we probably just need to widen es_processed and touch related
code. Not sure what else Vik saw that needed doing.
regards, tom lane
On Fri, Jan 31, 2014 at 04:38:21PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Fri, Jan 31, 2014 at 06:34:27PM +0100, Vik Fearing wrote: > >> Unfortunately, I gave up on it as being over my head when I noticed I > >> was changing the protocol itself. I should have notified the list so > >> someone else could have taken over. > > > OK, so that brings up a good question. Can we change the protocol for > > this without causing major breakage? Tom seems to indicate that it can > > be done for 9.4, but I thought protocol breakage was a major issue. Are > > we really changing the wire protocol here, or just the type of string we > > can pass back to the interface? > > What I said about it upthread was "this is effectively a protocol change, > albeit a pretty minor one, so I can't see back-patching it". > > The discussion in bug #7766 shows that some client-side code is likely to > need fixing, and that such fixing might actually be nontrivial for them. > So changing this in a minor release is clearly a bad idea. But I don't > have a problem with widening the counters in a major release where we > can document it as a potential compatibility issue. > > I took a quick look and noted that CMDSTATUS_LEN and > COMPLETION_TAG_BUFSIZE are set to 64, and have been for quite a long time, > so command status string buffer sizes should not be a problem. > > I think we probably just need to widen es_processed and touch related > code. Not sure what else Vik saw that needed doing. OK, thanks for the feedback. I understand now. The contents of the string will potentially have a larger integer, but the byte length of the string in the wire protocol doesn't change. Let's wait for Vik to reply and I think we can move forward. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 01/31/2014 10:56 PM, Bruce Momjian wrote: > On Fri, Jan 31, 2014 at 04:38:21PM -0500, Tom Lane wrote: >> Bruce Momjian <bruce@momjian.us> writes: >>> On Fri, Jan 31, 2014 at 06:34:27PM +0100, Vik Fearing wrote: >>>> Unfortunately, I gave up on it as being over my head when I noticed I >>>> was changing the protocol itself. I should have notified the list so >>>> someone else could have taken over. >>> OK, so that brings up a good question. Can we change the protocol for >>> this without causing major breakage? Tom seems to indicate that it can >>> be done for 9.4, but I thought protocol breakage was a major issue. Are >>> we really changing the wire protocol here, or just the type of string we >>> can pass back to the interface? >> What I said about it upthread was "this is effectively a protocol change, >> albeit a pretty minor one, so I can't see back-patching it". >> >> The discussion in bug #7766 shows that some client-side code is likely to >> need fixing, and that such fixing might actually be nontrivial for them. >> So changing this in a minor release is clearly a bad idea. But I don't >> have a problem with widening the counters in a major release where we >> can document it as a potential compatibility issue. >> >> I took a quick look and noted that CMDSTATUS_LEN and >> COMPLETION_TAG_BUFSIZE are set to 64, and have been for quite a long time, >> so command status string buffer sizes should not be a problem. >> >> I think we probably just need to widen es_processed and touch related >> code. Yes. >> Not sure what else Vik saw that needed doing. Quite a lot, actually. It seemed to me at the time to be a pretty big rabbit hole. > OK, thanks for the feedback. I understand now. The contents of the > string will potentially have a larger integer, but the byte length of > the string in the wire protocol doesn't change. > > Let's wait for Vik to reply and I think we can move forward. Unfortunately, I just did some cleanup last week and removed that branch. Had I waited a bit more I still would have had all the work I had done. I'll see how quickly I can redo it to get to the part where I got scared of what I was doing. It will have to wait until next week though; I am currently at FOSDEM. -- Vik
On Sat, Feb 1, 2014 at 02:25:16AM +0100, Vik Fearing wrote: > > OK, thanks for the feedback. I understand now. The contents of the > > string will potentially have a larger integer, but the byte length of > > the string in the wire protocol doesn't change. > > > > Let's wait for Vik to reply and I think we can move forward. > > Unfortunately, I just did some cleanup last week and removed that > branch. Had I waited a bit more I still would have had all the work I > had done. I'll see how quickly I can redo it to get to the part where I > got scared of what I was doing. > > It will have to wait until next week though; I am currently at FOSDEM. OK, thanks. I thought it only required passing the int64 around until it got into the string passed to the client. The original patch is in the email archives if you want it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On 02/01/2014 02:26 AM, Bruce Momjian wrote:
> On Sat, Feb 1, 2014 at 02:25:16AM +0100, Vik Fearing wrote:
>>> OK, thanks for the feedback. I understand now. The contents of the
>>> string will potentially have a larger integer, but the byte length of
>>> the string in the wire protocol doesn't change.
>>>
>>> Let's wait for Vik to reply and I think we can move forward.
>> Unfortunately, I just did some cleanup last week and removed that
>> branch. Had I waited a bit more I still would have had all the work I
>> had done. I'll see how quickly I can redo it to get to the part where I
>> got scared of what I was doing.
>>
>> It will have to wait until next week though; I am currently at FOSDEM.
> OK, thanks. I thought it only required passing the int64 around until
> it got into the string passed to the client. The original patch is in
> the email archives if you want it.
The original patch didn't have much in the way of actual work done,
unfortunately.
Without re-doing the work, my IRC logs show that I was bothered by this
in src/backend/tcop/postgres.c:
case 'E': /* execute */
{
const char *portal_name;
int max_rows;
forbidden_in_wal_sender(firstchar);
/* Set statement_timestamp() */
SetCurrentStatementStartTimestamp();
portal_name = pq_getmsgstring(&input_message);
max_rows = pq_getmsgint(&input_message, 4);
pq_getmsgend(&input_message);
exec_execute_message(portal_name, max_rows);
}
break;
I needed to change max_rows to int64 which meant I had to change
pq_getmsgint to pq_getmsgint64 which made me a little worried. I was
already overwhelmed by how much code I was changing and this one made me
reconsider.
If it's just a n00b thing, I guess I can pick this back up for 9.5.
--
Vik
Vik Fearing <vik.fearing@dalibo.com> writes:
> Without re-doing the work, my IRC logs show that I was bothered by this
> in src/backend/tcop/postgres.c:
> max_rows = pq_getmsgint(&input_message, 4);
> I needed to change max_rows to int64 which meant I had to change
> pq_getmsgint to pq_getmsgint64 which made me a little worried.
As well you should be, because we are *not* doing that. That would be
a guaranteed-incompatible protocol change. Fortunately, I don't see
any functional need for widening the row-limit field in execute messages;
how likely is it that someone wants to fetch exactly 3 billion rows?
The practical use-cases for nonzero row limits generally involve fetching
a bufferload worth of data at a time, so that the restriction to getting
no more than INT_MAX rows at once is several orders of magnitude away
from being a problem.
The same goes for internal uses of row limits, which makes it
questionable whether it's worth changing the width of ExecutorRun's
count parameter, which is what I assume you were on about here. But
in any case, if we did that we'd not try to reflect it as far as here,
because the message format specs can't change.
regards, tom lane
Did this every go any further?
I wrote some data transformation script at work, and after seeing "with count -2017657667" (and similar) in my scripts log I got a bit worried. seeing else where were folks just run a full on count(*) later to check counts but that is even MORE time and I was thinking it was a psycopg2 problem, but seems there are issues with the internal counters in pg as well for tracking "large" changes.
thanks,
Mark
On Sun, Feb 2, 2014 at 9:12 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Vik Fearing <vik.fearing@dalibo.com> writes:
> Without re-doing the work, my IRC logs show that I was bothered by this
> in src/backend/tcop/postgres.c:
> max_rows = pq_getmsgint(&input_message, 4);
> I needed to change max_rows to int64 which meant I had to change
> pq_getmsgint to pq_getmsgint64 which made me a little worried.
As well you should be, because we are *not* doing that. That would be
a guaranteed-incompatible protocol change. Fortunately, I don't see
any functional need for widening the row-limit field in execute messages;
how likely is it that someone wants to fetch exactly 3 billion rows?
The practical use-cases for nonzero row limits generally involve fetching
a bufferload worth of data at a time, so that the restriction to getting
no more than INT_MAX rows at once is several orders of magnitude away
from being a problem.
The same goes for internal uses of row limits, which makes it
questionable whether it's worth changing the width of ExecutorRun's
count parameter, which is what I assume you were on about here. But
in any case, if we did that we'd not try to reflect it as far as here,
because the message format specs can't change.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers