Thread: Re: Don't choke on files that are removed while pg_rewind runs.
commit b36805f3c54fe0e50e58bb9e6dad66daca46fbf6
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Sun Jun 28 21:35:51 2015 +0300
...
|@@ -175,22 +175,31 @@ libpqProcessFileList(void)
| pg_fatal("unexpected result set while fetching file list\n");
|
| /* Read result to local variables */
| for (i = 0; i < PQntuples(res); i++)
| {
| char *path = PQgetvalue(res, i, 0);
| int filesize = atoi(PQgetvalue(res, i, 1));
| bool isdir = (strcmp(PQgetvalue(res, i, 2), "t") == 0);
| char *link_target = PQgetvalue(res, i, 3);
| file_type_t type;
|
|+ if (PQgetisnull(res, 0, 1))
...
|+ continue;
Every other access to "res" in this loop is to res(i), which I believe is what
was intended here, too. Currently, it will dumbly loop but skip *every* row if
the 2nd column (1: size) of the first row (0) is null.
--
Justin
> On 13 Jul 2020, at 08:10, Justin Pryzby <pryzby@telsasoft.com> wrote: > Every other access to "res" in this loop is to res(i), which I believe is what > was intended here, too. Currently, it will dumbly loop but skip *every* row if > the 2nd column (1: size) of the first row (0) is null. Yeah, I agree with that, seems like the call should've been PQgetisnull(res, i, 1); to match the loop. cheers ./daniel
On Mon, 13 Jul 2020 at 15:34, Daniel Gustafsson <daniel@yesql.se> wrote: > > > On 13 Jul 2020, at 08:10, Justin Pryzby <pryzby@telsasoft.com> wrote: > > > Every other access to "res" in this loop is to res(i), which I believe is what > > was intended here, too. Currently, it will dumbly loop but skip *every* row if > > the 2nd column (1: size) of the first row (0) is null. > > Yeah, I agree with that, seems like the call should've been PQgetisnull(res, i, 1); > to match the loop. +1 Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jul 13, 2020 at 03:59:56PM +0900, Masahiko Sawada wrote: > On Mon, 13 Jul 2020 at 15:34, Daniel Gustafsson <daniel@yesql.se> wrote: >> Yeah, I agree with that, seems like the call should've been PQgetisnull(res, i, 1); >> to match the loop. > > +1 Good catch, Justin. There is a second thing here. The second column matches with the file size, so if its value is NULL then atol() would just crash first. I think that it would be more simple to first check if the file size is NULL (isdir and link_target would be also NULL, but just checking for the file size is fine), and then assign the result values, like in the attached. Any thoughts? -- Michael
Attachment
> On 13 Jul 2020, at 09:56, Michael Paquier <michael@paquier.xyz> wrote: > > On Mon, Jul 13, 2020 at 03:59:56PM +0900, Masahiko Sawada wrote: >> On Mon, 13 Jul 2020 at 15:34, Daniel Gustafsson <daniel@yesql.se> wrote: >>> Yeah, I agree with that, seems like the call should've been PQgetisnull(res, i, 1); >>> to match the loop. >> >> +1 > > Good catch, Justin. There is a second thing here. The second column > matches with the file size, so if its value is NULL then atol() would > just crash first. Does it? PGgetvalue will return an empty string and not NULL, so atol will convert that to zero wont it? It can be argued whether zero is the right size for a missing file, but it shouldn't crash at least. > I think that it would be more simple to first check > if the file size is NULL (isdir and link_target would be also NULL, > but just checking for the file size is fine), and then assign the > result values, like in the attached. Any thoughts? It does convey the meaning of code to do it after, since the data isn't useful in case the filesize is zero, but I don't have strong feelings for/against. Question is, rather than discard rows pulled from the server, should the query be tweaked to not include it in the first place instead? cheers ./daniel
On Mon, Jul 13, 2020 at 10:12:54AM +0200, Daniel Gustafsson wrote: > Does it? PGgetvalue will return an empty string and not NULL, so atol will > convert that to zero wont it? It can be argued whether zero is the right size > for a missing file, but it shouldn't crash at least. Nay, you are right. Thanks. > It does convey the meaning of code to do it after, since the data isn't useful > in case the filesize is zero, but I don't have strong feelings for/against. > Question is, rather than discard rows pulled from the server, should the query > be tweaked to not include it in the first place instead? That sounds like a good idea with an extra qual in the first part of the inner CTE, if coupled with a check to make sure that we never get a NULL result. Now there is IMO an argument to not complicate more this query as it is not like a lot of tuples would get filtered out anyway because of a NULL set of values? I don't have strong feelings for one approach or the other, but if I were to choose, I would just let the code as-is, without the change in the CTE. -- Michael
Attachment
> On 13 Jul 2020, at 14:18, Michael Paquier <michael@paquier.xyz> wrote: > That sounds like a good idea with an extra qual in the first part of > the inner CTE, if coupled with a check to make sure that we never > get a NULL result. Now there is IMO an argument to not complicate > more this query as it is not like a lot of tuples would get filtered > out anyway because of a NULL set of values? I don't have strong > feelings for one approach or the other, but if I were to choose, I > would just let the code as-is, without the change in the CTE. I don't have strong opinions either, both approaches will work, so feel free to go ahead with the proposed change. cheers ./daniel
On Tue, Jul 14, 2020 at 12:18:41PM +0200, Daniel Gustafsson wrote: > I don't have strong opinions either, both approaches will work, so feel free to > go ahead with the proposed change. Thanks. I have just gone with the solution to not change the query, and applied it down to 9.5. Please note that I also maintain an older version for 9.4 and 9.3, that has the same problem: https://github.com/vmware/pg_rewind So I'll go fix it. -- Michael