Horton

Thread

Commits

  1. Fix handling of missing files when using pg_rewind with online source

  2. Don't choke on files that are removed while pg_rewind runs.

  1. Re: Don't choke on files that are removed while pg_rewind runs.

    Justin Pryzby <pryzby@telsasoft.com> — 2020-07-13T06:10:10Z

    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

  2. Re: Don't choke on files that are removed while pg_rewind runs.

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-13T06:34:06Z

    > 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

  3. Re: Don't choke on files that are removed while pg_rewind runs.

    Masahiko Sawada <masahiko.sawada@2ndquadrant.com> — 2020-07-13T06:59:56Z

    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

  4. Re: Don't choke on files that are removed while pg_rewind runs.

    Michael Paquier <michael@paquier.xyz> — 2020-07-13T07:56:29Z

    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

  5. Re: Don't choke on files that are removed while pg_rewind runs.

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-13T08:12:54Z

    > 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

  6. Re: Don't choke on files that are removed while pg_rewind runs.

    Michael Paquier <michael@paquier.xyz> — 2020-07-13T12:18:24Z

    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

  7. Re: Don't choke on files that are removed while pg_rewind runs.

    Daniel Gustafsson <daniel@yesql.se> — 2020-07-14T10:18:41Z

    > 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

  8. Re: Don't choke on files that are removed while pg_rewind runs.

    Michael Paquier <michael@paquier.xyz> — 2020-07-15T06:25:51Z

    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