8000 Fix some issues with WAL segment opening for pg_receivewal --compress · postgres/postgres@7fbe0c8 · GitHub
[go: up one dir, main page]
Skip to content

Commit 7fbe0c8

Browse files
michaelpqmichaelpq
committed
Fix some issues with WAL segment opening for pg_receivewal --compress
The logic handling the opening of new WAL segments was fuzzy when using --compress if a partial, non-compressed, segment with the same base name existed in the repository storing those files. In this case, using --compress would cause the code to first check for the existence and the size of a non-compressed segment, followed by the opening of a new compressed, partial, segment. The code was accidentally working correctly on most platforms as the buildfarm has proved, except bowerbird where gzflush() could fail in this code path. It is wrong anyway to take the code path used pre-padding when creating a new partial, non-compressed, segment, so let's fix it. Note that this issue exists when users mix successive runs of pg_receivewal with or without compression, as discovered with the tests introduced by ffc9dda. While on it, this refactors the code so as code paths that need to know about the ".gz" suffix are down from four to one in walmethods.c, easing a bit the introduction of new compression methods. This addresses a second issue where log messages generated for an unexpected failure would not show the compressed segment name involved, which was confusing, printing instead the name of the non-compressed equivalent. Reported-by: Georgios Kokolatos Discussion: https://postgr.es/m/YPDLz2x3o1aX2wRh@paquier.xyz Backpatch-through: 10
1 parent 01c3adc commit 7fbe0c8

File tree

3 files changed

+78
-23
lines changed

3 files changed

+78
-23
lines changed

‎src/bin/pg_basebackup/receivelog.c

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,29 @@ static bool
8888
open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
8989
{
9090
Walfile *f;
91-
char fn[MAXPGPATH];
91+
char *fn;
9292
ssize_t size;
9393
XLogSegNo segno;
9494

9595
XLByteToSeg(startpoint, segno, WalSegSz);
9696
XLogFileName(current_walfile_name, stream->timeline, segno, WalSegSz);
9797

98-
snprintf(fn, sizeof(fn), "%s%s", current_walfile_name,
99-
stream->partial_suffix ? stream->partial_suffix : "");
98+
/* Note that this considers the compression used if necessary */
99+
fn = stream->walmethod->get_file_name(current_walfile_name,
100+
stream->partial_suffix);
100101

101102
/*
102103
* When streaming to files, if an existing file exists we verify that it's
103104
* either empty (just created), or a complete WalSegSz segment (in which
104105
* case it has been created and padded). Anything else indicates a corrupt
105-
* file.
106+
* file. Compressed files have no need for padding, so just ignore this
107+
* case.
106108
*
107109
* When streaming to tar, no file with this name will exist before, so we
108110
* never have to verify a size.
109111
*/
110-
if (stream->walmethod->existsfile(fn))
112+
if (stream->walmethod->compression() == 0 &&
113+
stream->walmethod->existsfile(fn))
111114
{
112115
size = stream->walmethod->get_file_size(fn);
113116
if (size < 0)

‎src/bin/pg_basebackup/walmethods.c

Lines changed: 61 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -68,20 +68,32 @@ dir_getlasterror(void)
6868
return strerror(errno);
6969
}
7070

71+
static char *
72+
dir_get_file_name(const char *pathname, const char *temp_suffix)
73+
{
74+
char *filename = pg_malloc0(MAXPGPATH * sizeof(char));
75+
76+
snprintf(filename, MAXPGPATH, "%s%s%s",
77+
pathname, dir_data->compression > 0 ? ".gz" : "",
78+
temp_suffix ? temp_suffix : "");
79+
80+
return filename;
81+
}
82+
7183
static Walfile
7284
dir_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
7385
{
7486
static char tmppath[MAXPGPATH];
87+
char *filename;
7588
int fd;
7689
DirectoryMethodFile *f;
7790
#ifdef HAVE_LIBZ
7891
gzFile gzfp = NULL;
7992
#endif
8093

81-
snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
82-
dir_data->basedir, pathname,
83-
dir_data->compression > 0 ? ".gz" : "",
84-
temp_suffix ? temp_suffix : "");
94+
filename = dir_get_file_name(pathname, temp_suffix);
95+
snprintf(tmppath, sizeof(tmppath), "%s/%s",
96+
dir_data->basedir, filename);
8597

8698
/*
8799
* Open a file for non-compressed as well as compressed files. Tracking
@@ -232,26 +244,31 @@ dir_close(Walfile f, WalCloseMethod method)
232244
/* Build path to the current version of the file */
233245
if (method == CLOSE_NORMAL && df->temp_suffix)
234246
{
247+
char *filename;
248+
char *filename2;
249+
235250
/*
236251
* If we have a temp prefix, normal operation is to rename the
237252
* file.
238253
*/
239-
snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
240-
dir_data->basedir, df->pathname,
241-
dir_data->compression > 0 ? ".gz" : "",
242-
df->temp_suffix);
243-
snprintf(tmppath2, sizeof(tmppath2), "%s/%s%s",
244-
dir_data->basedir, df->pathname,
245-
dir_data->compression > 0 ? ".gz" : "");
254+
filename = dir_get_file_name(df->pathname, df->temp_suffix);
255+
snprintf(tmppath, sizeof(tmppath), "%s/%s",
256+
dir_data->basedir, filename);
257+
258+
/* permanent name, so no need for the prefix */
259+
filename2 = dir_get_file_name(df->pathname, NULL);
260+
snprintf(tmppath2, sizeof(tmppath2), "%s/%s",
261+
dir_data->basedir, filename2);
246262
r = durable_rename(tmppath, tmppath2);
247263
}
248264
else if (method == CLOSE_UNLINK)
249265
{
266+
char *filename;
267+
250268
/* Unlink the file once it's closed */
251-
snprintf(tmppath, sizeof(tmppath), "%s/%s%s%s",
252-
dir_data->basedir, df->pathname,
253-
dir_data->compression > 0 ? ".gz" : "",
254-
df->temp_suffix ? df->temp_suffix : "");
269+
filename = dir_get_file_name(df->pathname, df->temp_suffix);
270+
snprintf(tmppath, sizeof(tmppath F438 ), "%s/%s",
271+
dir_data->basedir, filename);
255272
r = unlink(tmppath);
256273
}
257274
else
@@ -313,6 +330,12 @@ dir_get_file_size(const char *pathname)
313330
return statbuf.st_size;
314331
}
315332

333+
static int
334+
dir_compression(void)
335+
{
336+
return dir_data->compression;
337+
}
338+
316339
static bool
317340
dir_existsfile(const char *pathname)
318341
{
@@ -355,6 +378,8 @@ CreateWalDirectoryMethod(const char *basedir, int compression, bool sync)
355378
method->write = dir_write;
356379
method->get_current_pos = dir_get_current_pos;
357380
method->get_file_size = dir_get_file_size;
381+
method->get_file_name = dir_get_file_name;
382+
method->compression = dir_compression;
358383
method->close = dir_close;
359384
method->sync = dir_sync;
360385
method->existsfile = dir_existsfile;
@@ -527,11 +552,22 @@ tar_write_padding_data(TarMethodFile *f, size_t bytes)
527552
return true;
528553
}
529554

555+
static char *
556+
tar_get_file_name(const char *pathname, const char *temp_suffix)
557+
{
558+
char *filename = pg_malloc0(MAXPGPATH * sizeof(char));
559+
560+
snprintf(filename, MAXPGPATH, "%s%s",
561+
pathname, temp_suffix ? temp_suffix : "");
562+
563+
return filename;
564+
}
565+
530566
static Walfile
531567
tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_size)
532568
{
533569
int save_errno;
534-
static char tmppath[MAXPGPATH];
570+
char *tmppath;
535571

536572
tar_clear_error();
537573

@@ -583,8 +619,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
583619

584620
tar_data->currentfile = pg_malloc0(sizeof(TarMethodFile));
585621

586-
snprintf(tmppath, sizeof(tmppath), "%s%s",
587-
pathname, temp_suffix ? temp_suffix : "");
622+
tmppath = tar_get_file_name(pathname, temp_suffix);
588623

589624
/* Create a header with size set to 0 - we will fill out the size on close */
590625
if (tarCreateHeader(tar_data->currentfile->header, tmppath, NULL, 0, S_IRUSR | S_IWUSR, 0, 0, time(NULL)) != TAR_OK)
@@ -689,6 +724,12 @@ tar_get_file_size(const char *pathname)
689724
return -1;
690725
}
691726

727+
static int
728+
tar_compression(void)
729+
{
730+
return tar_data->compression;
731+
}
732+
692733
static off_t
693734
tar_get_current_pos(Walfile f)
694735
{
@@ -994,6 +1035,8 @@ CreateWalTarMethod(const char *tarbase, int compression, bool sync)
9941035
method->write = tar_write;
9951036
method->get_current_pos = tar_get_current_pos;
9961037
method->get_file_size = tar_get_file_size;
1038+
method->get_file_name = tar_get_file_name;
1039+
method->compression = tar_compression;
9971040
method->close = tar_close;
9981041
method->sync = tar_sync;
9991042
method->existsfile = tar_existsfile;

‎src/bin/pg_basebackup/walmethods.h

9856
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,15 @@ struct WalWriteMethod
5252
/* Return the size of a file, or -1 on failure. */
5353
ssize_t (*get_file_size) (const char *pathname);
5454

55+
/*
56+
* Return the name of the current file to work on, without the base
57+
* directory. This is useful for logging.
58+
*/
59+
char *(*get_file_name) (const char *pathname, const char *temp_suffix);
60+
61+
/* Return the level of compression */
62+
int (*compression) (void);
63+
5564
/*
5665
* Write count number of bytes to the file, and return the number of bytes
5766
* actually written or -1 for error.

0 commit comments

Comments
 (0)
0