Thread
Commits
-
Fix inconsistency in determining the timestamp of the db statfile.
- 03c7f1f37a1f 14.0 landed
-
Split pgstat file in smaller pieces
- 187492b6c2e8 9.3.0 cited
-
Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-08T06:10:18Z
We use the timestamp of the global statfile if we are not able to determine it for a particular database either because the entry for that database doesn't exist or there is an error while reading the specific database entry. This was not taken care of while reading other entries like ArchiverStats or SLRUStats. See pgstat_read_db_statsfile_timestamp. I have observed this while reviewing Sawada-San's patch related to logical replication stats [1]. I think this can only happen if due to some reason the statfile got corrupt or we have some bug in stats writing code, the chances of both are rare and even if that happens we will use stale statistics. The attached patch by Sawada-San will fix this issue. As the chances of this the problem is rare and nobody has reported any issue related to this, so it might be okay not to backpatch this. Thoughts? [1] - https://www.postgresql.org/message-id/CAA4eK1JBqQh9cBKjO-nKOOE%3D7f6ONDCZp0TJZfn4VsQqRZ%2BuYA%40mail.gmail.com -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-08T10:28:02Z
On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > We use the timestamp of the global statfile if we are not able to > determine it for a particular database either because the entry for > that database doesn't exist or there is an error while reading the > specific database entry. This was not taken care of while reading > other entries like ArchiverStats or SLRUStats. See > pgstat_read_db_statsfile_timestamp. I have observed this while > reviewing Sawada-San's patch related to logical replication stats [1]. > > I think this can only happen if due to some reason the statfile got > corrupt or we > have some bug in stats writing code, the chances of both are rare and even > if that happens we will use stale statistics. > > The attached patch by Sawada-San will fix this issue. As the chances of > this > the problem is rare and nobody has reported any issue related to this, > so it might be okay not to backpatch this. > > Thoughts? > > Why are we reading the archiver statis and and slru stats in "pgstat_read_db_statsfile_timestamp" in the first place? That seems well out of scope for that function. If nothing else the comment at the top of that function is out of touch with reality. We do seem to read it into local buffers and then ignore the contents. I guess we're doing it just to verify that it's not corrupt -- so perhaps the function should actually have a different name than it does now, since it certainly seems to do more than actually read the statsfile timestamp. Specifically in this patch it looks wrong -- in the case of say the SLRU stats being corrupt, we will now return the timestamp of the global stats file even if there is one existing for the database requested, which definitely breaks the contract of the function. It's already broken, but this doesn't seem to make it less so. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
-
Re: Inconsistency in determining the timestamp of the db statfile.
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-09-08T13:11:01Z
On 2020/09/08 19:28, Magnus Hagander wrote: > > > On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: > > We use the timestamp of the global statfile if we are not able to > determine it for a particular database either because the entry for > that database doesn't exist or there is an error while reading the > specific database entry. This was not taken care of while reading > other entries like ArchiverStats or SLRUStats. See > pgstat_read_db_statsfile_timestamp. I have observed this while > reviewing Sawada-San's patch related to logical replication stats [1]. > > I think this can only happen if due to some reason the statfile got > corrupt or we > have some bug in stats writing code, the chances of both are rare and even > if that happens we will use stale statistics. > > The attached patch by Sawada-San will fix this issue. As the chances of this > the problem is rare and nobody has reported any issue related to this, > so it might be okay not to backpatch this. > > Thoughts? > > > Why are we reading the archiver statis and and slru stats in "pgstat_read_db_statsfile_timestamp" in the first place? Maybe because they are written before database stats entries? That is, to read the target database stats entry and get the timestamp from it, we need to read (or lseek) all the global stats entries written before them. > That seems well out of scope for that function. > > If nothing else the comment at the top of that function is out of touch with reality. We do seem to read it into local buffers and then ignore the contents. I guess we're doing it just to verify that it's not corrupt -- so perhaps the function should actually have a different name than it does now, since it certainly seems to do more than actually read the statsfile timestamp. > > Specifically in this patch it looks wrong -- in the case of say the SLRU stats being corrupt, we will now return the timestamp of the global stats file even if there is one existing for the database requested, which definitely breaks the contract of the function. Yes. We should return false when fread() for database entry fails, instead? That is, 1. If corrupted stats file is found, the function always returns false. 2. If the file is not currupted and the target database entry is found, its timestamp is returned. 3. If the file is not corrupted and the target is NOT found, the timestamp of global entry is returned. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
-
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-08T13:32:52Z
On Tue, Sep 8, 2020 at 3:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > On 2020/09/08 19:28, Magnus Hagander wrote: > > > > > > On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com > <mailto:amit.kapila16@gmail.com>> wrote: > > > > We use the timestamp of the global statfile if we are not able to > > determine it for a particular database either because the entry for > > that database doesn't exist or there is an error while reading the > > specific database entry. This was not taken care of while reading > > other entries like ArchiverStats or SLRUStats. See > > pgstat_read_db_statsfile_timestamp. I have observed this while > > reviewing Sawada-San's patch related to logical replication stats > [1]. > > > > I think this can only happen if due to some reason the statfile got > > corrupt or we > > have some bug in stats writing code, the chances of both are rare > and even > > if that happens we will use stale statistics. > > > > The attached patch by Sawada-San will fix this issue. As the chances > of this > > the problem is rare and nobody has reported any issue related to > this, > > so it might be okay not to backpatch this. > > > > Thoughts? > > > > > > Why are we reading the archiver statis and and slru stats in > "pgstat_read_db_statsfile_timestamp" in the first place? > > Maybe because they are written before database stats entries? That is, > to read the target database stats entry and get the timestamp from it, > we need to read (or lseek) all the global stats entries written before > them. > > Oh meh. Yeah, I'm reading this thing completely wrong :/ Ignore my comments :) > That seems well out of scope for that function. > > > > If nothing else the comment at the top of that function is out of touch > with reality. We do seem to read it into local buffers and then ignore the > contents. I guess we're doing it just to verify that it's not corrupt -- so > perhaps the function should actually have a different name than it does > now, since it certainly seems to do more than actually read the statsfile > timestamp. > > > > Specifically in this patch it looks wrong -- in the case of say the SLRU > stats being corrupt, we will now return the timestamp of the global stats > file even if there is one existing for the database requested, which > definitely breaks the contract of the function. > > Yes. > We should return false when fread() for database entry fails, instead? > That is, > > 1. If corrupted stats file is found, the function always returns false. > 2. If the file is not currupted and the target database entry is found, > its timestamp is returned. > 3. If the file is not corrupted and the target is NOT found, the timestamp > of global entry is returned > Yeah, with more coffee and re-reading it, I'm not sure how we could have the database stats being OK if the archiver or slru stats are not. That said, I think it still makes sense to return false if the stats file is corrupt. How much can we trust the first block, if the block right after it is corrupt? So yeah, I think your described order seems correct. But that's also what the code already did before this patch, isn't it? -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
-
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-09T03:04:03Z
On Tue, Sep 8, 2020 at 7:03 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Tue, Sep 8, 2020 at 3:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> >> >> >> On 2020/09/08 19:28, Magnus Hagander wrote: >> > >> > >> > On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: >> > >> > We use the timestamp of the global statfile if we are not able to >> > determine it for a particular database either because the entry for >> > that database doesn't exist or there is an error while reading the >> > specific database entry. This was not taken care of while reading >> > other entries like ArchiverStats or SLRUStats. See >> > pgstat_read_db_statsfile_timestamp. I have observed this while >> > reviewing Sawada-San's patch related to logical replication stats [1]. >> > >> > I think this can only happen if due to some reason the statfile got >> > corrupt or we >> > have some bug in stats writing code, the chances of both are rare and even >> > if that happens we will use stale statistics. >> > >> > The attached patch by Sawada-San will fix this issue. As the chances of this >> > the problem is rare and nobody has reported any issue related to this, >> > so it might be okay not to backpatch this. >> > >> > Thoughts? >> > >> > >> > Why are we reading the archiver statis and and slru stats in "pgstat_read_db_statsfile_timestamp" in the first place? >> >> Maybe because they are written before database stats entries? That is, >> to read the target database stats entry and get the timestamp from it, >> we need to read (or lseek) all the global stats entries written before them. >> > > Oh meh. Yeah, I'm reading this thing completely wrong :/ Ignore my comments :) > > > >> > That seems well out of scope for that function. >> > >> > If nothing else the comment at the top of that function is out of touch with reality. We do seem to read it into local buffers and then ignore the contents. I guess we're doing it just to verify that it's not corrupt -- so perhaps the function should actually have a different name than it does now, since it certainly seems to do more than actually read the statsfile timestamp. >> > >> > Specifically in this patch it looks wrong -- in the case of say the SLRU stats being corrupt, we will now return the timestamp of the global stats file even if there is one existing for the database requested, which definitely breaks the contract of the function. >> >> Yes. >> We should return false when fread() for database entry fails, instead? That is, >> >> 1. If corrupted stats file is found, the function always returns false. >> 2. If the file is not currupted and the target database entry is found, its timestamp is returned. >> 3. If the file is not corrupted and the target is NOT found, the timestamp of global entry is returned > > > Yeah, with more coffee and re-reading it, I'm not sure how we could have the database stats being OK if the archiver or slru stats are not. > > That said, I think it still makes sense to return false if the stats file is corrupt. How much can we trust the first block, if the block right after it is corrupt? So yeah, I think your described order seems correct. But that's also what the code already did before this patch, isn't it? > No, before patch as well, if we can't read the DB entry say because the file is corrupt, we return true and use timestamp of global stats file and this is what is established by the original commit 187492b6. So, if we consider that as correct then the functionality is something like once we have read the timestamp of the global statfile, we use that if we can't read the actual db entry for whatever reason. It seems if we return false, the caller will call this function again in the loop. Now, I see the point that if we can't read some part of the file we should return false instead but not sure if that is helpful from the perspective of the caller of pgstat_read_db_statsfile_timestamp. I have included Alvaro as he is a committer for 187492b6, so he might remember something and let us know if this is a mistake or there is some reason for doing so (return true even when the db entry we are trying to read is corrupt). -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-09-09T05:24:25Z
On 2020/09/09 12:04, Amit Kapila wrote: > On Tue, Sep 8, 2020 at 7:03 PM Magnus Hagander <magnus@hagander.net> wrote: >> >> On Tue, Sep 8, 2020 at 3:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >>> >>> >>> >>> On 2020/09/08 19:28, Magnus Hagander wrote: >>>> >>>> >>>> On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: >>>> >>>> We use the timestamp of the global statfile if we are not able to >>>> determine it for a particular database either because the entry for >>>> that database doesn't exist or there is an error while reading the >>>> specific database entry. This was not taken care of while reading >>>> other entries like ArchiverStats or SLRUStats. See >>>> pgstat_read_db_statsfile_timestamp. I have observed this while >>>> reviewing Sawada-San's patch related to logical replication stats [1]. >>>> >>>> I think this can only happen if due to some reason the statfile got >>>> corrupt or we >>>> have some bug in stats writing code, the chances of both are rare and even >>>> if that happens we will use stale statistics. >>>> >>>> The attached patch by Sawada-San will fix this issue. As the chances of this >>>> the problem is rare and nobody has reported any issue related to this, >>>> so it might be okay not to backpatch this. >>>> >>>> Thoughts? >>>> >>>> >>>> Why are we reading the archiver statis and and slru stats in "pgstat_read_db_statsfile_timestamp" in the first place? >>> >>> Maybe because they are written before database stats entries? That is, >>> to read the target database stats entry and get the timestamp from it, >>> we need to read (or lseek) all the global stats entries written before them. >>> >> >> Oh meh. Yeah, I'm reading this thing completely wrong :/ Ignore my comments :) >> >> >> >>>> That seems well out of scope for that function. >>>> >>>> If nothing else the comment at the top of that function is out of touch with reality. We do seem to read it into local buffers and then ignore the contents. I guess we're doing it just to verify that it's not corrupt -- so perhaps the function should actually have a different name than it does now, since it certainly seems to do more than actually read the statsfile timestamp. >>>> >>>> Specifically in this patch it looks wrong -- in the case of say the SLRU stats being corrupt, we will now return the timestamp of the global stats file even if there is one existing for the database requested, which definitely breaks the contract of the function. >>> >>> Yes. >>> We should return false when fread() for database entry fails, instead? That is, >>> >>> 1. If corrupted stats file is found, the function always returns false. >>> 2. If the file is not currupted and the target database entry is found, its timestamp is returned. >>> 3. If the file is not corrupted and the target is NOT found, the timestamp of global entry is returned >> >> >> Yeah, with more coffee and re-reading it, I'm not sure how we could have the database stats being OK if the archiver or slru stats are not. >> >> That said, I think it still makes sense to return false if the stats file is corrupt. How much can we trust the first block, if the block right after it is corrupt? So yeah, I think your described order seems correct. But that's also what the code already did before this patch, isn't it? >> > > No, before patch as well, if we can't read the DB entry say because > the file is corrupt, we return true and use timestamp of global stats > file and this is what is established by the original commit 187492b6. > So, if we consider that as correct then the functionality is something > like once we have read the timestamp of the global statfile, we use > that if we can't read the actual db entry for whatever reason. It > seems if we return false, the caller will call this function again in > the loop. Now, I see the point that if we can't read some part of the > file we should return false instead but not sure if that is helpful > from the perspective of the caller of > pgstat_read_db_statsfile_timestamp. When false is returned, the caller backend_read_statsfile() seems to request the stats collector to write a fresh stats data into the file, and then pgstat_read_db_statsfile_timestamp() can try to read the fresh file that may not be corrupted. So returning false in that case seems ok to me... Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
-
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-09T07:11:23Z
On Wed, Sep 9, 2020 at 10:54 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > On 2020/09/09 12:04, Amit Kapila wrote: > > > > No, before patch as well, if we can't read the DB entry say because > > the file is corrupt, we return true and use timestamp of global stats > > file and this is what is established by the original commit 187492b6. > > So, if we consider that as correct then the functionality is something > > like once we have read the timestamp of the global statfile, we use > > that if we can't read the actual db entry for whatever reason. It > > seems if we return false, the caller will call this function again in > > the loop. Now, I see the point that if we can't read some part of the > > file we should return false instead but not sure if that is helpful > > from the perspective of the caller of > > pgstat_read_db_statsfile_timestamp. > > When false is returned, the caller backend_read_statsfile() seems to > request the stats collector to write a fresh stats data into the file, > and then pgstat_read_db_statsfile_timestamp() can try to read the fresh > file that may not be corrupted. So returning false in that case seems ok > to me... > Hmm, the request to get fresh data is sent irrespective of true or false. We send it to get the latest data if it is not present and it is not guaranteed that the request will lead to a write of stats file. So, I am not sure if we can override that with the corrupted file case, sure there is something to it but if we really want to rely on that mechanism we should explicitly send such a request on detection of a corrupted file. -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-09T09:45:20Z
On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Tue, Sep 8, 2020 at 7:03 PM Magnus Hagander <magnus@hagander.net> > wrote: > > > > On Tue, Sep 8, 2020 at 3:11 PM Fujii Masao <masao.fujii@oss.nttdata.com> > wrote: > >> > >> > >> > >> On 2020/09/08 19:28, Magnus Hagander wrote: > >> > > >> > > >> > On Tue, Sep 8, 2020 at 8:10 AM Amit Kapila <amit.kapila16@gmail.com > <mailto:amit.kapila16@gmail.com>> wrote: > >> > > >> > We use the timestamp of the global statfile if we are not able to > >> > determine it for a particular database either because the entry > for > >> > that database doesn't exist or there is an error while reading the > >> > specific database entry. This was not taken care of while reading > >> > other entries like ArchiverStats or SLRUStats. See > >> > pgstat_read_db_statsfile_timestamp. I have observed this while > >> > reviewing Sawada-San's patch related to logical replication stats > [1]. > >> > > >> > I think this can only happen if due to some reason the statfile > got > >> > corrupt or we > >> > have some bug in stats writing code, the chances of both are rare > and even > >> > if that happens we will use stale statistics. > >> > > >> > The attached patch by Sawada-San will fix this issue. As the > chances of this > >> > the problem is rare and nobody has reported any issue related to > this, > >> > so it might be okay not to backpatch this. > >> > > >> > Thoughts? > >> > > >> > > >> > Why are we reading the archiver statis and and slru stats in > "pgstat_read_db_statsfile_timestamp" in the first place? > >> > >> Maybe because they are written before database stats entries? That is, > >> to read the target database stats entry and get the timestamp from it, > >> we need to read (or lseek) all the global stats entries written before > them. > >> > > > > Oh meh. Yeah, I'm reading this thing completely wrong :/ Ignore my > comments :) > > > > > > > >> > That seems well out of scope for that function. > >> > > >> > If nothing else the comment at the top of that function is out of > touch with reality. We do seem to read it into local buffers and then > ignore the contents. I guess we're doing it just to verify that it's not > corrupt -- so perhaps the function should actually have a different name > than it does now, since it certainly seems to do more than actually read > the statsfile timestamp. > >> > > >> > Specifically in this patch it looks wrong -- in the case of say the > SLRU stats being corrupt, we will now return the timestamp of the global > stats file even if there is one existing for the database requested, which > definitely breaks the contract of the function. > >> > >> Yes. > >> We should return false when fread() for database entry fails, instead? > That is, > >> > >> 1. If corrupted stats file is found, the function always returns false. > >> 2. If the file is not currupted and the target database entry is found, > its timestamp is returned. > >> 3. If the file is not corrupted and the target is NOT found, the > timestamp of global entry is returned > > > > > > Yeah, with more coffee and re-reading it, I'm not sure how we could have > the database stats being OK if the archiver or slru stats are not. > > > > That said, I think it still makes sense to return false if the stats > file is corrupt. How much can we trust the first block, if the block right > after it is corrupt? So yeah, I think your described order seems correct. > But that's also what the code already did before this patch, isn't it? > > > > No, before patch as well, if we can't read the DB entry say because > the file is corrupt, we return true and use timestamp of global stats > file and this is what is established by the original commit 187492b6. > Uh, the patch changes: - return false; + return true; In the "codepath of corruption". After also setting the value. So AFAICT before the patch, it returns false if the file is corrupt (there's a set of such scenarios, all returning false), just after logging that it's corrupt.The patch changes it to log that it's corrupt and then return true. The fact that it doesn't find the database doesn't mean the file is corrupt, it just means the database is inactive. But missing global, archiver or slru stats means it's corrupt. > So, if we consider that as correct then the functionality is something > like once we have read the timestamp of the global statfile, we use > that if we can't read the actual db entry for whatever reason. It > seems if we return false, the caller will call this function again in > the loop. Now, I see the point that if we can't read some part of the > file we should return false instead but not sure if that is helpful > from the perspective of the caller of > pgstat_read_db_statsfile_timestamp. > But we are not talking about the "if we can't read the actual db entry" case, we are talking about the *global* parts of the file. Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. By returning false in the corrupt case we make the caller sleep and try again, which seems to be the correct thing to do (since the stats collector will be rewriting the file regularly) I have included Alvaro as he is a committer for 187492b6, so he might > remember something and let us know if this is a mistake or there is > some reason for doing so (return true even when the db entry we are > trying to read is corrupt). > Again, it looks to me like 187492b6 is doing exactly that -- it returns false on corrupt file. It returns true if the file is OK, regardless of if it finds the database, using the global timestamp if the database is not found. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>
-
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-09T09:46:56Z
On Wed, Sep 9, 2020 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Wed, Sep 9, 2020 at 10:54 AM Fujii Masao <masao.fujii@oss.nttdata.com> > wrote: > > > > On 2020/09/09 12:04, Amit Kapila wrote: > > > > > > No, before patch as well, if we can't read the DB entry say because > > > the file is corrupt, we return true and use timestamp of global stats > > > file and this is what is established by the original commit 187492b6. > > > So, if we consider that as correct then the functionality is something > > > like once we have read the timestamp of the global statfile, we use > > > that if we can't read the actual db entry for whatever reason. It > > > seems if we return false, the caller will call this function again in > > > the loop. Now, I see the point that if we can't read some part of the > > > file we should return false instead but not sure if that is helpful > > > from the perspective of the caller of > > > pgstat_read_db_statsfile_timestamp. > > > > When false is returned, the caller backend_read_statsfile() seems to > > request the stats collector to write a fresh stats data into the file, > > and then pgstat_read_db_statsfile_timestamp() can try to read the fresh > > file that may not be corrupted. So returning false in that case seems ok > > to me... > > > > Hmm, the request to get fresh data is sent irrespective of true or > false. We send it to get the latest data if it is not present and it > No we don't. Just before we request it, there is: /* Normal acceptance case: file is not older than cutoff time */ if (ok && file_ts >= min_ts) break; So it only requests a new file in the case that it returned false. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/> -
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-09T10:23:40Z
On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> > > Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. > Yeah, this is exactly the case I was pointing out where we return true before the patch, basically the code below: case 'D': if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), fpin) != offsetof(PgStat_StatDBEntry, tables)) { ereport(pgStatRunningInCollector ? LOG : WARNING, (errmsg("corrupted statistics file \"%s\"", statfile))); goto done; } done: FreeFile(fpin); return true; Now, if we decide to return 'false' here, then surely there is no argument and we should return false in other cases as well. Basically, I think we should be consistent in handling the corrupt file case. -- With Regards, Amit Kapila. -
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-09T10:26:51Z
On Wed, Sep 9, 2020 at 3:17 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Wed, Sep 9, 2020 at 9:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Wed, Sep 9, 2020 at 10:54 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: >> > >> > On 2020/09/09 12:04, Amit Kapila wrote: >> > > >> > > No, before patch as well, if we can't read the DB entry say because >> > > the file is corrupt, we return true and use timestamp of global stats >> > > file and this is what is established by the original commit 187492b6. >> > > So, if we consider that as correct then the functionality is something >> > > like once we have read the timestamp of the global statfile, we use >> > > that if we can't read the actual db entry for whatever reason. It >> > > seems if we return false, the caller will call this function again in >> > > the loop. Now, I see the point that if we can't read some part of the >> > > file we should return false instead but not sure if that is helpful >> > > from the perspective of the caller of >> > > pgstat_read_db_statsfile_timestamp. >> > >> > When false is returned, the caller backend_read_statsfile() seems to >> > request the stats collector to write a fresh stats data into the file, >> > and then pgstat_read_db_statsfile_timestamp() can try to read the fresh >> > file that may not be corrupted. So returning false in that case seems ok >> > to me... >> > >> >> Hmm, the request to get fresh data is sent irrespective of true or >> false. We send it to get the latest data if it is not present and it > > > No we don't. Just before we request it, there is: > /* Normal acceptance case: file is not older than cutoff time */ > if (ok && file_ts >= min_ts) > break; > > So it only requests a new file in the case that it returned false. > What if the second part of the above 'if' statement is false, then basically even when pgstat_read_db_statsfile_timestamp() has returned true, we can send a request. IIUC, here the basic idea is if the stats in the file is updated before cut_off time, then we do send the request and wait for updated stats. -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2020-09-09T13:56:42Z
On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote: >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net> wrote: >> >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >>> >> >> Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. >> > >Yeah, this is exactly the case I was pointing out where we return true >before the patch, basically the code below: >case 'D': >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), > fpin) != offsetof(PgStat_StatDBEntry, tables)) >{ >ereport(pgStatRunningInCollector ? LOG : WARNING, >(errmsg("corrupted statistics file \"%s\"", >statfile))); >goto done; >} > >done: >FreeFile(fpin); >return true; > >Now, if we decide to return 'false' here, then surely there is no >argument and we should return false in other cases as well. Basically, >I think we should be consistent in handling the corrupt file case. > FWIW I do agree with this - we should return false here, to make it return false like in the other data corruption cases. AFAICS that's the only inconsistency here. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-09T13:57:36Z
On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote: > >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net> > wrote: > >> > >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com> > wrote: > >>> > >> > >> Though in fact the one inconsistent place in the code now is that if it > is corrupt in the db entry part of the file it returns true and the global > timestamp, which I would argue is perhaps incorrect and it should return > false. > >> > > > >Yeah, this is exactly the case I was pointing out where we return true > >before the patch, basically the code below: > >case 'D': > >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), > > fpin) != offsetof(PgStat_StatDBEntry, tables)) > >{ > >ereport(pgStatRunningInCollector ? LOG : WARNING, > >(errmsg("corrupted statistics file \"%s\"", > >statfile))); > >goto done; > >} > > > >done: > >FreeFile(fpin); > >return true; > > > >Now, if we decide to return 'false' here, then surely there is no > >argument and we should return false in other cases as well. Basically, > >I think we should be consistent in handling the corrupt file case. > > > > FWIW I do agree with this - we should return false here, to make it > return false like in the other data corruption cases. AFAICS that's the > only inconsistency here. > +1, I think that's the place to fix, rather than reversing all the other places. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/> -
Re: Inconsistency in determining the timestamp of the db statfile.
Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-09T16:01:26Z
On 2020-Sep-09, Amit Kapila wrote: > I have included Alvaro as he is a committer for 187492b6, so he might > remember something and let us know if this is a mistake or there is > some reason for doing so (return true even when the db entry we are > trying to read is corrupt). Thanks -- I have to excuse myself here, as I don't have too many memories about this. It seems y'all have derived more insight that I could possibly offer. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Inconsistency in determining the timestamp of the db statfile.
Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-09-09T16:07:40Z
On 2020/09/09 22:57, Magnus Hagander wrote: > On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote: > > On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote: > >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net <mailto:magnus@hagander.net>> wrote: > >> > >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: > >>> > >> > >> Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. > >> > > > >Yeah, this is exactly the case I was pointing out where we return true > >before the patch, basically the code below: > >case 'D': > >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), > > fpin) != offsetof(PgStat_StatDBEntry, tables)) > >{ > >ereport(pgStatRunningInCollector ? LOG : WARNING, > >(errmsg("corrupted statistics file \"%s\"", > >statfile))); > >goto done; > >} > > > >done: > >FreeFile(fpin); > >return true; > > > >Now, if we decide to return 'false' here, then surely there is no > >argument and we should return false in other cases as well. Basically, > >I think we should be consistent in handling the corrupt file case. > > > > FWIW I do agree with this - we should return false here, to make it > return false like in the other data corruption cases. AFAICS that's the > only inconsistency here. > > > +1, I think that's the place to fix, rather than reversing all the other places. +1 as I suggested upthread! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION -
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-10T05:24:30Z
On Wed, Sep 9, 2020 at 9:37 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > On 2020/09/09 22:57, Magnus Hagander wrote: > > On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote: > > > > On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote: > > >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net <mailto:magnus@hagander.net>> wrote: > > >> > > >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: > > >>> > > >> > > >> Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. > > >> > > > > > >Yeah, this is exactly the case I was pointing out where we return true > > >before the patch, basically the code below: > > >case 'D': > > >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), > > > fpin) != offsetof(PgStat_StatDBEntry, tables)) > > >{ > > >ereport(pgStatRunningInCollector ? LOG : WARNING, > > >(errmsg("corrupted statistics file \"%s\"", > > >statfile))); > > >goto done; > > >} > > > > > >done: > > >FreeFile(fpin); > > >return true; > > > > > >Now, if we decide to return 'false' here, then surely there is no > > >argument and we should return false in other cases as well. Basically, > > >I think we should be consistent in handling the corrupt file case. > > > > > > > FWIW I do agree with this - we should return false here, to make it > > return false like in the other data corruption cases. AFAICS that's the > > only inconsistency here. > > > > > > +1, I think that's the place to fix, rather than reversing all the other places. > > +1 as I suggested upthread! > Please find the patch attached based on the above discussion. I have slightly adjusted the comments. -- With Regards, Amit Kapila. -
Re: Inconsistency in determining the timestamp of the db statfile.
Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-09-10T06:21:41Z
On Thu, 10 Sep 2020 at 14:24, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Wed, Sep 9, 2020 at 9:37 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/09/09 22:57, Magnus Hagander wrote: > > > On Wed, Sep 9, 2020 at 3:56 PM Tomas Vondra <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote: > > > > > > On Wed, Sep 09, 2020 at 03:53:40PM +0530, Amit Kapila wrote: > > > >On Wed, Sep 9, 2020 at 3:15 PM Magnus Hagander <magnus@hagander.net <mailto:magnus@hagander.net>> wrote: > > > >> > > > >> On Wed, Sep 9, 2020 at 5:04 AM Amit Kapila <amit.kapila16@gmail.com <mailto:amit.kapila16@gmail.com>> wrote: > > > >>> > > > >> > > > >> Though in fact the one inconsistent place in the code now is that if it is corrupt in the db entry part of the file it returns true and the global timestamp, which I would argue is perhaps incorrect and it should return false. > > > >> > > > > > > > >Yeah, this is exactly the case I was pointing out where we return true > > > >before the patch, basically the code below: > > > >case 'D': > > > >if (fread(&dbentry, 1, offsetof(PgStat_StatDBEntry, tables), > > > > fpin) != offsetof(PgStat_StatDBEntry, tables)) > > > >{ > > > >ereport(pgStatRunningInCollector ? LOG : WARNING, > > > >(errmsg("corrupted statistics file \"%s\"", > > > >statfile))); > > > >goto done; > > > >} > > > > > > > >done: > > > >FreeFile(fpin); > > > >return true; > > > > > > > >Now, if we decide to return 'false' here, then surely there is no > > > >argument and we should return false in other cases as well. Basically, > > > >I think we should be consistent in handling the corrupt file case. > > > > > > > > > > FWIW I do agree with this - we should return false here, to make it > > > return false like in the other data corruption cases. AFAICS that's the > > > only inconsistency here. > > > > > > > > > +1, I think that's the place to fix, rather than reversing all the other places. > > > > +1 as I suggested upthread! > > > > Please find the patch attached based on the above discussion. I have > slightly adjusted the comments. FWIW reading the discussion, I also agree to fix it to return false in case of file corruption. Regarding the v2 patch, I think we should return false in the following case too: default: ereport(pgStatRunningInCollector ? LOG : WARNING, (errmsg("corrupted statistics file \"%s\"", statfile))); goto done; Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-10T07:05:01Z
On Thu, Sep 10, 2020 at 11:52 AM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote: > > > Regarding the v2 patch, I think we should return false in the > following case too: > > default: > ereport(pgStatRunningInCollector ? LOG : WARNING, > (errmsg("corrupted statistics file \"%s\"", > statfile))); > goto done; > makes sense, attached find the updated patch. -- With Regards, Amit Kapila. -
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-10T07:33:43Z
On Thu, Sep 10, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Sep 10, 2020 at 11:52 AM Masahiko Sawada > <masahiko.sawada@2ndquadrant.com> wrote: > > > > > > Regarding the v2 patch, I think we should return false in the > > following case too: > > > > default: > > ereport(pgStatRunningInCollector ? LOG : WARNING, > > (errmsg("corrupted statistics file \"%s\"", > > statfile))); > > goto done; > > > > makes sense, attached find the updated patch. > As a minor nitpick, technically, I think the comment change is wrong, because it says that the caller *must* rely on the timestamp, which it of course doesn't. I think a more proper one is "The caller must not rely on the timestamp in case the function returns false" or "The caller must only rely on the timestamp if the function returns true". +1 on the code parts. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/> -
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-10T07:43:18Z
On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Thu, Sep 10, 2020 at 9:05 AM Amit Kapila <amit.kapila16@gmail.com> wrote: >> >> On Thu, Sep 10, 2020 at 11:52 AM Masahiko Sawada >> <masahiko.sawada@2ndquadrant.com> wrote: >> > >> > >> > Regarding the v2 patch, I think we should return false in the >> > following case too: >> > >> > default: >> > ereport(pgStatRunningInCollector ? LOG : WARNING, >> > (errmsg("corrupted statistics file \"%s\"", >> > statfile))); >> > goto done; >> > >> >> makes sense, attached find the updated patch. > > > As a minor nitpick, technically, I think the comment change is wrong, because it says that the caller *must* rely on the timestamp, which it of course doesn't. I think a more proper one is "The caller must not rely on the timestamp in case the function returns false" or "The caller must only rely on the timestamp if the function returns true". > The comments already say what you said in the second suggestion:"The caller must rely on timestamp stored in *ts iff the function returns true.". Read iff "as if and only if" > +1 on the code parts. > BTW, do we want to backpatch this? There is no user reported bug and not sure if the user will encounter any problem. I think it is a minor improvement and more of code consistency. So, making HEAD only change should be okay. -- With Regards, Amit Kapila. -
Re: Inconsistency in determining the timestamp of the db statfile.
Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-09-10T13:12:10Z
On 2020-Sep-10, Amit Kapila wrote: > On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <magnus@hagander.net> wrote: > The comments already say what you said in the second suggestion:"The > caller must rely on timestamp stored in *ts iff the function returns > true.". Read iff "as if and only if" I think "must" should be "may" there, if we're nitpicking. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-11T02:52:56Z
On Thu, Sep 10, 2020 at 6:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2020-Sep-10, Amit Kapila wrote: > > > On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <magnus@hagander.net> wrote: > > > The comments already say what you said in the second suggestion:"The > > caller must rely on timestamp stored in *ts iff the function returns > > true.". Read iff "as if and only if" > > I think "must" should be "may" there, if we're nitpicking. > Here, we want to say that "caller can rely on *ts only if the function returns true". If we replace 'must' with 'may' then it seems to me we are trying to say that caller can ignore the timestamp value, if so, why at first place caller has called this function. -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Amit Kapila <amit.kapila16@gmail.com> — 2020-09-12T11:49:24Z
On Thu, Sep 10, 2020 at 1:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > BTW, do we want to backpatch this? There is no user reported bug and > not sure if the user will encounter any problem. I think it is a minor > improvement and more of code consistency. So, making HEAD only change > should be okay. > Seeing no other opinions, pushed this in Head. -- With Regards, Amit Kapila.
-
Re: Inconsistency in determining the timestamp of the db statfile.
Magnus Hagander <magnus@hagander.net> — 2020-09-12T13:17:58Z
On Fri, Sep 11, 2020 at 4:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > On Thu, Sep 10, 2020 at 6:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> > wrote: > > > > On 2020-Sep-10, Amit Kapila wrote: > > > > > On Thu, Sep 10, 2020 at 1:03 PM Magnus Hagander <magnus@hagander.net> > wrote: > > > > > The comments already say what you said in the second suggestion:"The > > > caller must rely on timestamp stored in *ts iff the function returns > > > true.". Read iff "as if and only if" > > > > I think "must" should be "may" there, if we're nitpicking. > > > > Here, we want to say that "caller can rely on *ts only if the function > returns true". If we replace 'must' with 'may' then it seems to me we > are trying to say that caller can ignore the timestamp value, if so, > why at first place caller has called this function. > This is true, but that should really be the decision of the caller, not of the function. But again, that's extremely nitpicky, so it doesn't really matter :) And +1 on the push you did, and the decision not to backpatch it since there haven't been any reports. -- Magnus Hagander Me: https://www.hagander.net/ <http://www.hagander.net/> Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>