Updated backup APIs for non-exclusive backups

On Wed, Feb 10, 2016 at 7:46 AM, Magnus Hagander <magnus@hagander.net> wrote:

Per discussionat the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup. The idea is to fix at least three main issues
that are there today -- that you cannot run concurrent backups, that the
backup_label file is created in the data directory which makes it impossible
to distinguish between a cluster restored from backup and one that crashed
while a backup was running, and a cluster can get "stuck" in backup mode if
the backup script/software crashes.

To make this work, this patch:

* Introduces a new argument for pg_start_backup(), for "exclusive". It
defaults to true, which means that just calling pg_start_backup() like
before will behave exactly like before. If it's called with exclusive='f', a
non-exclusive backup is started, and the backup label is collected in memory
instead of in the backup_label file.

* If the client disconnects with a non-exclusive backup running, the backup
is automatically aborted. This is the same thing that pg_basebackup does. To
use these non-exclusive backups the backup software will have to maintain a
persistent connection to the database -- something that should not be a
problem for any of the modern ones out there (but would've been slightly
trickier back in the days when we suggested shellscripts)

* A new version of pg_stop_backup is created, taking an argument specifying
if it's exclusive or not. The current version of pg_stop_backup() continues
to work for exclusive backups, with no changes to behavior. The new
pg_stop_backup will return a record of three columns instead of just the
value -- the LSN (pglsn), the backup label file (text) and the tablespace
map file (text). If used to cancel an exclusive backup, backup label file
and tablespace map will be NULL. At this point it's up to the backup
software to write down the files in the location of the backup.

Attached patch currently doesn't include full documentation, since Bruce was
going to restructure that section of the docs (also based on the
devmeeting). I will write up the docs once that is done (I assume it will be
soon enough, or I'll go do it regardless), but I wanted to get some review
in on the code while waiting.

Wow, this is a great idea.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:

Per discussionat the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup.

Thanks for following through with this!

* If the client disconnects with a non-exclusive backup running, the backup
is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

* A new version of pg_stop_backup is created, taking an argument specifying
if it's exclusive or not. The current version of pg_stop_backup() continues
to work for exclusive backups, with no changes to behavior. The new
pg_stop_backup will return a record of three columns instead of just the
value -- the LSN (pglsn), the backup label file (text) and the tablespace
map file (text).

I wonder if we shouldn't change the API a bit more aggressively. Right
now we return the label and the map - but what if there's more files at
some later point? One way would be to make it a SETOF function returning
'filename', 'content' or such. Makes it less clear what to do with the
lsn admittedly.

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:

Per discussionat the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup.

Thanks for following through with this!

* If the client disconnects with a non-exclusive backup running, the

backup

is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

Not sure what you're referring to here. Do you mean being able to make a
non-exclusive backup while not maintaining a connection? That's going to
make things a *lot* more complicated, as we'd have to invent something like
"backup slots" similar to what we're doing with replication slots. I doubt
it's worth all that work and complexity.

* A new version of pg_stop_backup is created, taking an argument

specifying

if it's exclusive or not. The current version of pg_stop_backup()

continues

to work for exclusive backups, with no changes to behavior. The new
pg_stop_backup will return a record of three columns instead of just the
value -- the LSN (pglsn), the backup label file (text) and the tablespace
map file (text).

I wonder if we shouldn't change the API a bit more aggressively. Right
now we return the label and the map - but what if there's more files at
some later point? One way would be to make it a SETOF function returning
'filename', 'content' or such. Makes it less clear what to do with the
lsn admittedly.

*Adding* more columns to the API shouldn't be a problem in the future. If
there's another file, we can return a fourth column. A backup program is
going to have to know about those things anyway and shouldn't just blindly
write those files to the backup, IMO.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

* Andres Freund (andres@anarazel.de) wrote:

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:

* If the client disconnects with a non-exclusive backup running, the backup
is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

I was thinking along the same lines.

* A new version of pg_stop_backup is created, taking an argument specifying
if it's exclusive or not. The current version of pg_stop_backup() continues
to work for exclusive backups, with no changes to behavior. The new
pg_stop_backup will return a record of three columns instead of just the
value -- the LSN (pglsn), the backup label file (text) and the tablespace
map file (text).

I wonder if we shouldn't change the API a bit more aggressively. Right
now we return the label and the map - but what if there's more files at
some later point? One way would be to make it a SETOF function returning
'filename', 'content' or such. Makes it less clear what to do with the
lsn admittedly.

If we make the 'client disconnect means abort' optional then we'd also
need to modify the API of stop backup to specify which backup to stop,
I'd think.

Thanks!

Stephen

* Magnus Hagander (magnus@hagander.net) wrote:

On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:

Per discussionat the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup.

Thanks for following through with this!

* If the client disconnects with a non-exclusive backup running, the

backup

is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

Not sure what you're referring to here. Do you mean being able to make a
non-exclusive backup while not maintaining a connection? That's going to
make things a *lot* more complicated, as we'd have to invent something like
"backup slots" similar to what we're doing with replication slots. I doubt
it's worth all that work and complexity.

Hrmmm. If that's the case then perhaps you're right. I liked the
general idea of not having to maintain a TCP connection during the
entire backup (TCP connections can be annoyingly finicky in certain
environments...), but I'm not sure it's worth a lot of additional
complexity.

Thanks!

Stephen

On 2/10/16 9:44 AM, Stephen Frost wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Wed, Feb 10, 2016 at 2:46 PM, Andres Freund <andres@anarazel.de> wrote:

On 2016-02-10 13:46:05 +0100, Magnus Hagander wrote:

* If the client disconnects with a non-exclusive backup running, the

backup

is automatically aborted. This is the same thing that pg_basebackup does.
To use these non-exclusive backups the backup software will have to
maintain a persistent connection to the database -- something that should
not be a problem for any of the modern ones out there (but would've been
slightly trickier back in the days when we suggested shellscripts)

I think we might want to make this one optional, but I'm not going to
fight super hard for it.

Not sure what you're referring to here. Do you mean being able to make a
non-exclusive backup while not maintaining a connection? That's going to
make things a *lot* more complicated, as we'd have to invent something like
"backup slots" similar to what we're doing with replication slots. I doubt
it's worth all that work and complexity.

Agreed.

Hrmmm. If that's the case then perhaps you're right. I liked the
general idea of not having to maintain a TCP connection during the
entire backup (TCP connections can be annoyingly finicky in certain
environments...), but I'm not sure it's worth a lot of additional
complexity.

Well, pgBackRest holds a connection to PostgreSQL through the entire
backup and will abort the backup if it is severed. The connection is
always held locally, though, even if the master backup process is on a
different server. I haven't gotten any reports of aborted backups due
to the connection failing.

--
-David
david@pgmasters.net

* David Steele (david@pgmasters.net) wrote:

On 2/10/16 9:44 AM, Stephen Frost wrote:

Hrmmm. If that's the case then perhaps you're right. I liked the
general idea of not having to maintain a TCP connection during the
entire backup (TCP connections can be annoyingly finicky in certain
environments...), but I'm not sure it's worth a lot of additional
complexity.

Well, pgBackRest holds a connection to PostgreSQL through the entire
backup and will abort the backup if it is severed. The connection is
always held locally, though, even if the master backup process is on a
different server. I haven't gotten any reports of aborted backups due
to the connection failing.

Yeah, I know, I had been thinking it might be nice to not do that at
some point in the future, but thinking on it further, we've already got
a "pick up where you left off" capability with pgbackrest, so it's
really not a huge deal if the backup fails and has to be restarted, and
this does remove the need (or at least deprecate) to use the "stop an
already-running backup if you find one" option that pgbackrest has.

Thanks!

Stephen

On 2/10/16 7:46 AM, Magnus Hagander wrote:

Per discussion at the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup. <...>

This sounds like a great idea and I have signed up to review.

* A new version of pg_stop_backup is created, taking an argument
specifying if it's exclusive or not. The current version of
pg_stop_backup() continues to work for exclusive backups, with no
changes to behavior. The new pg_stop_backup will return a record of
three columns instead of just the value -- the LSN (pglsn), the backup
label file (text) and the tablespace map file (text). If used to cancel
an exclusive backup, backup label file and tablespace map will be NULL.
At this point it's up to the backup software to write down the files in
the location of the backup.

It would be nice if this new pg_stop_backup() function also output the
exact time when the backup stopped. Depending on how long
pg_stop_backup() waits for WAL to be archived a time-stamp recorded
after pg_stop_backup() returns can be pretty far off.

This information is handy for automating selection of a backup when
doing time-based PITR (or so the user can manually select). For
exclusive backups it is possible to parse the .backup file to get this
information but non-exclusive backups do not output the .backup file.

I would be happy to see the time-stamp returned from the
pg_start_backup() function as well. It's a bigger change, but once
pg_start_backup() returns multiple columns it will be easier to add more
columns in the future.

In fact, it might be better if backup_label and tablespace_map were
output by pg_start_backup(). This would give the backup software more
information to work with from the start.

Thanks!
--
-David
david@pgmasters.net

On Wed, Feb 10, 2016 at 4:38 PM, David Steele <david@pgmasters.net> wrote:

On 2/10/16 7:46 AM, Magnus Hagander wrote:

Per discussion at the developer meeting in Brussels, here's a patch that
makes some updates to the backup APIs, to support non-exclusive backups
without using pg_basebackup. <...>

This sounds like a great idea and I have signed up to review.

* A new version of pg_stop_backup is created, taking an argument
specifying if it's exclusive or not. The current version of
pg_stop_backup() continues to work for exclusive backups, with no
changes to behavior. The new pg_stop_backup will return a record of
three columns instead of just the value -- the LSN (pglsn), the backup
label file (text) and the tablespace map file (text). If used to cancel
an exclusive backup, backup label file and tablespace map will be NULL.
At this point it's up to the backup software to write down the files in
the location of the backup.

It would be nice if this new pg_stop_backup() function also output the
exact time when the backup stopped. Depending on how long
pg_stop_backup() waits for WAL to be archived a time-stamp recorded
after pg_stop_backup() returns can be pretty far off.

This information is handy for automating selection of a backup when
doing time-based PITR (or so the user can manually select). For
exclusive backups it is possible to parse the .backup file to get this
information but non-exclusive backups do not output the .backup file.

The non-exclusive backups *do* output the backup_label file, it just shows
up as a text field instead of as a separate file. You're supposed to write
that data to a file in the backup program.

I would be happy to see the time-stamp returned from the
pg_start_backup() function as well. It's a bigger change, but once
pg_start_backup() returns multiple columns it will be easier to add more
columns in the future.

In fact, it might be better if backup_label and tablespace_map were
output by pg_start_backup(). This would give the backup software more
information to work with from the start.

I was trying to figure out why that's a bad idea, but I can't really say
why :)

For pg_basebackup backups, I think the reason we put it at the end is
simply that we don't want to write it into the backup directory until the
rest of the backup is complete, since it's not going to be useful before
then. But that requirement can certainly be put on the backup client
instead of the server. With the newer API it's going to have to keep those
things around anyway.

That would then change the function syntax for pg_start_backup() but
instead make pg_stop_backup() now behave the same as it did before.

Would anybody else like to comment on if that's a good idea or if there are
any holes in it? :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On 2016-02-10 16:50:26 +0100, Magnus Hagander wrote:

I would be happy to see the time-stamp returned from the
pg_start_backup() function as well. It's a bigger change, but once
pg_start_backup() returns multiple columns it will be easier to add more
columns in the future.

In fact, it might be better if backup_label and tablespace_map were
output by pg_start_backup(). This would give the backup software more
information to work with from the start.

I was trying to figure out why that's a bad idea, but I can't really say
why :)

For pg_basebackup backups, I think the reason we put it at the end is
simply that we don't want to write it into the backup directory until the
rest of the backup is complete, since it's not going to be useful before
then. But that requirement can certainly be put on the backup client
instead of the server. With the newer API it's going to have to keep those
things around anyway.

That would then change the function syntax for pg_start_backup() but
instead make pg_stop_backup() now behave the same as it did before.

Would anybody else like to comment on if that's a good idea or if there are
any holes in it? :)

I don't think that's a good idea. It makes it impossible to add
information to labels about the minimum recovery point and
such. Currently there's some rather fragile logic to discover that, but
I'd really like to get rid of that at some point.

Regards,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/10/16 11:01 AM, Andres Freund wrote:

On 2016-02-10 16:50:26 +0100, Magnus Hagander wrote:

I would be happy to see the time-stamp returned from the
pg_start_backup() function as well. It's a bigger change, but once
pg_start_backup() returns multiple columns it will be easier to add more
columns in the future.

In fact, it might be better if backup_label and tablespace_map were
output by pg_start_backup(). This would give the backup software more
information to work with from the start.

I was trying to figure out why that's a bad idea, but I can't really say
why :)

For pg_basebackup backups, I think the reason we put it at the end is
simply that we don't want to write it into the backup directory until the
rest of the backup is complete, since it's not going to be useful before
then. But that requirement can certainly be put on the backup client
instead of the server. With the newer API it's going to have to keep those
things around anyway.

That would then change the function syntax for pg_start_backup() but
instead make pg_stop_backup() now behave the same as it did before.

Would anybody else like to comment on if that's a good idea or if there are
any holes in it? :)

I don't think that's a good idea. It makes it impossible to add
information to labels about the minimum recovery point and
such. Currently there's some rather fragile logic to discover that, but
I'd really like to get rid of that at some point.

That makes sense. The backup_label "as is" could be output at the
beginning but if we want to add the minimum recovery point it would need
to be output at the end.

It seems like tablespace_map could still be output at the beginning, though.

--
-David
david@pgmasters.net

On 2/10/16 10:50 AM, Magnus Hagander wrote:

On Wed, Feb 10, 2016 at 4:38 PM, David Steele <david@pgmasters.net

This information is handy for automating selection of a backup when
doing time-based PITR (or so the user can manually select). For
exclusive backups it is possible to parse the .backup file to get this
information but non-exclusive backups do not output the .backup file.

The non-exclusive backups *do* output the backup_label file, it just
shows up as a text field instead of as a separate file. You're supposed
to write that data to a file in the backup program.

I meant the .backup file (e.g. 000000010000008C0000001A.00000028.backup)
that is archived along with the WAL for an exlcusive backup. I believe
this is currently the only place to get the stop time (without reading
the WAL segments).

--
-David
david@pgmasters.net

On 2016-02-10 11:06:01 -0500, David Steele wrote:

That makes sense. The backup_label "as is" could be output at the
beginning but if we want to add the minimum recovery point it would need
to be output at the end.

It seems like tablespace_map could still be output at the beginning, though.

I don't really see enough benefit to go that way. What are you thinking
of using the information for ("This would give the backup software more
information to work with from the start.")?

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2/10/16 11:12 AM, Andres Freund wrote:

On 2016-02-10 11:06:01 -0500, David Steele wrote:

That makes sense. The backup_label "as is" could be output at the
beginning but if we want to add the minimum recovery point it would need
to be output at the end.

It seems like tablespace_map could still be output at the beginning, though.

I don't really see enough benefit to go that way. What are you thinking
of using the information for ("This would give the backup software more
information to work with from the start.")?

Currently backup software that handles tablespaces needs to read the
pg_tblspc directory to build an oid/path mapping in order to know which
tablespace directories to copy. Since Postgres is already doing this
when it creates tablespace_map it seems like it's redundant for the
backup software to do it again.

Since tablespace_map is created in do_pg_start_backup() and I don't see
how it could be updated after that, I think it is logical to output it
from pg_start_backup(). I don't feel strongly about it, though.

--
-David
david@pgmasters.net

Hi Magnus,

thanks for working on this topic.
What it does is very similar to what Barman's pgespresso extension does and I'd like to see it implemented soon in the core.

I've added myself as reviewer for the patch on commitfest site.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

Hi Magnus,

I've finally found some time to take a look to the patch.

It applies with some fuzziness on master, but the result looks correct.
Unfortunately the OID of the new pg_stop_backup function conflicts with
"pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).

After changing it the patch does not compile:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -g -O0 -g -fno-omit-frame-pointer
-I../../../../src/include
-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/libxml2
-I/usr/local/include -I/usr/local/opt/openssl/include -c -o xlog.o
xlog.c -MMD -MP -MF .deps/xlog.Po
xlog.c:10000:19: error: use of undeclared identifier 'tblspc_mapfbuf';
did you mean 'tblspcmapfile'?
initStringInfo(&tblspc_mapfbuf);
^~~~~~~~~~~~~~
tblspcmapfile
xlog.c:9790:19: note: 'tblspcmapfile' declared here
StringInfo tblspcmapfile, bool infotbssize,
^
xlog.c:10073:22: error: use of undeclared identifier 'tblspc_mapfbuf';
did you mean 'tblspcmapfile'?
appendStringInfo(&tblspc_mapfbuf, "%s %s\n", ti->oid, ti->path);
^~~~~~~~~~~~~~
tblspcmapfile
xlog.c:9790:19: note: 'tblspcmapfile' declared here
StringInfo tblspcmapfile, bool infotbssize,
^
xlog.c:10092:19: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
initStringInfo(&labelfbuf);
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10099:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "START WAL LOCATION: %X/%X (file %s)\n",
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10101:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "CHECKPOINT LOCATION: %X/%X\n",
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10103:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "BACKUP METHOD: %s\n",
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10105:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "BACKUP FROM: %s\n",
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10107:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "START TIME: %s\n", strfbuf);
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10108:21: error: use of undeclared identifier 'labelfbuf'; did
you mean 'labelfile'?
appendStringInfo(&labelfbuf, "LABEL: %s\n", backupidstr);
^~~~~~~~~
labelfile
xlog.c:9789:19: note: 'labelfile' declared here
StringInfo labelfile, DIR *tblspcdir, List **tablespaces,
^
xlog.c:10142:15: error: use of undeclared identifier 'labelfbuf'
if (fwrite(labelfbuf.data, labelfbuf.len, 1, fp) != 1 ||
^
xlog.c:10142:31: error: use of undeclared identifier 'labelfbuf'
if (fwrite(labelfbuf.data, labelfbuf.len, 1, fp) != 1 ||
^
xlog.c:10151:10: error: use of undeclared identifier 'labelfbuf'
pfree(labelfbuf.data);
^
xlog.c:10154:8: error: use of undeclared identifier 'tblspc_mapfbuf'
if (tblspc_mapfbuf.len > 0)
^
xlog.c:10178:16: error: use of undeclared identifier 'tblspc_mapfbuf'
if (fwrite(tblspc_mapfbuf.data, tblspc_mapfbuf.len, 1, fp) != 1 ||
^
xlog.c:10178:37: error: use of undeclared identifier 'tblspc_mapfbuf'
if (fwrite(tblspc_mapfbuf.data, tblspc_mapfbuf.len, 1, fp) != 1 ||
^
xlog.c:10189:10: error: use of undeclared identifier 'tblspc_mapfbuf'
pfree(tblspc_mapfbuf.data);
^
xlog.c:10193:17: error: use of undeclared identifier 'labelfbuf'
*labelfile = labelfbuf.data;
^
xlog.c:10194:8: error: use of undeclared identifier 'tblspc_mapfbuf'
if (tblspc_mapfbuf.len > 0)
^
xlog.c:10195:22: error: use of undeclared identifier 'tblspc_mapfbuf'
*tblspcmapfile = tblspc_mapfbuf.data;
^
19 errors generated.
make[4]: *** [xlog.o] Error 1
make[3]: *** [transam-recursive] Error 2
make[2]: *** [access-recursive] Error 2
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2

I've searched in past commits for any recent change that involves the
affected lines, but I have not found any.
Maybe some changes are missing?

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

Hi Magnus,

On 3/2/16 12:49 PM, Marco Nenciarini wrote:

I've finally found some time to take a look to the patch.

It applies with some fuzziness on master, but the result looks correct.
Unfortunately the OID of the new pg_stop_backup function conflicts with
"pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).

After changing it the patch does not compile:

It's been two weeks since Marco found these issues in the patch. Please
provide an updated patch soon or I will mark this "returned with feedback".

Thanks,
--
-David
david@pgmasters.net

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Mar 16, 2016 at 5:06 PM, David Steele <david@pgmasters.net> wrote:

Hi Magnus,

On 3/2/16 12:49 PM, Marco Nenciarini wrote:

I've finally found some time to take a look to the patch.

It applies with some fuzziness on master, but the result looks correct.
Unfortunately the OID of the new pg_stop_backup function conflicts with
"pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).

After changing it the patch does not compile:

It's been two weeks since Marco found these issues in the patch. Please
provide an updated patch soon or I will mark this "returned with feedback".

Hi!

Thanks for the note - I had missed Marco's response completely!

I'll take a look at it once Nordic PGDay is done!

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Wed, Mar 2, 2016 at 6:49 PM, Marco Nenciarini <
marco.nenciarini@2ndquadrant.it> wrote:

Hi Magnus,

Hi!

First, again my apologies for completely missing that you had posted this
review!

I've finally found some time to take a look to the patch.

It applies with some fuzziness on master, but the result looks correct.
Unfortunately the OID of the new pg_stop_backup function conflicts with
"pg_blocking_pids()" patch (52f5d578d6c29bf254e93c69043b817d4047ca67).

Fixed, thanks!

After changing it the patch does not compile:

It compiles fine for me, and with no warnings.

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute
-Wformat-security -fno-strict-aliasing -fwrapv
-Wno-unused-command-line-argument -g -O0 -g -fno-omit-frame-pointer
-I../../../../src/include

-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk/usr/include/libxml2
-I/usr/local/include -I/usr/local/opt/openssl/include -c -o xlog.o
xlog.c -MMD -MP -MF .deps/xlog.Po
xlog.c:10000:19: error: use of undeclared identifier 'tblspc_mapfbuf';

Eh. There is no presence of "tblspc_mapfbuf" after the patch. I think it
looks like the "applies with fuzziness" actually wasn't correct, and you
ended up with bad code with a mix of the old and the new code in it.

I've attached an updated patch, which is rebased on current master and
includes the oid fix.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/