public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 08/20] union{} with a CompressionAlgorithm alg
4+ messages / 4 participants
[nested] [flat]
* [PATCH 08/20] union{} with a CompressionAlgorithm alg
@ 2020-12-21 06:11 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Justin Pryzby @ 2020-12-21 06:11 UTC (permalink / raw)
---
src/bin/pg_dump/compress_io.c | 200 ++++++++++++++++++----------------
src/bin/pg_dump/pg_dump.c | 2 +-
2 files changed, 106 insertions(+), 96 deletions(-)
diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c
index fa94148cdf..e07436bc21 100644
--- a/src/bin/pg_dump/compress_io.c
+++ b/src/bin/pg_dump/compress_io.c
@@ -651,23 +651,27 @@ WriteDataToArchiveNone(ArchiveHandle *AH, CompressorState *cs,
*/
struct cfp
{
- FILE *uncompressedfp;
+ CompressionAlgorithm alg;
+
+ union {
+ FILE *fp;
+
#ifdef HAVE_LIBZ
- gzFile compressedfp;
+ gzFile gzfp;
#endif
-#ifdef HAVE_LIBZSTD // XXX: this should be a union with a CompressionAlgorithm alg?
- /* This is a normal file to which we read/write compressed data */
- struct {
- FILE *fp;
- // XXX: use one separate ZSTD_CStream per thread: disable on windows ?
- ZSTD_CStream *cstream;
- ZSTD_DStream *dstream;
- ZSTD_outBuffer output;
- ZSTD_inBuffer input;
- } zstd;
+#ifdef HAVE_LIBZSTD
+ struct {
+ /* This is a normal file to which we read/write compressed data */
+ FILE *fp;
+ // XXX: use one separate ZSTD_CStream per thread: disable on windows ?
+ ZSTD_CStream *cstream;
+ ZSTD_DStream *dstream;
+ ZSTD_outBuffer output;
+ ZSTD_inBuffer input;
+ } zstd;
#endif
-
+ } u;
};
static int hasSuffix(const char *filename);
@@ -754,6 +758,8 @@ cfopen(const char *path, const char *mode, Compress *compression)
{
cfp *fp = pg_malloc0(sizeof(cfp));
+ fp->alg = compression->alg;
+
switch (compression->alg)
{
#ifdef HAVE_LIBZ
@@ -765,15 +771,15 @@ cfopen(const char *path, const char *mode, Compress *compression)
snprintf(mode_compression, sizeof(mode_compression), "%s%d",
mode, compression->level);
- fp->compressedfp = gzopen(path, mode_compression);
+ fp->u.gzfp = gzopen(path, mode_compression);
}
else
{
/* don't specify a level, just use the zlib default */
- fp->compressedfp = gzopen(path, mode);
+ fp->u.gzfp = gzopen(path, mode);
}
- if (fp->compressedfp == NULL)
+ if (fp->u.gzfp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -783,8 +789,8 @@ cfopen(const char *path, const char *mode, Compress *compression)
#ifdef HAVE_LIBZSTD
case COMPR_ALG_ZSTD:
- fp->zstd.fp = fopen(path, mode);
- if (fp->zstd.fp == NULL)
+ fp->u.zstd.fp = fopen(path, mode);
+ if (fp->u.zstd.fp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -792,23 +798,23 @@ cfopen(const char *path, const char *mode, Compress *compression)
else if (mode[0] == 'w' || mode[0] == 'a' ||
strchr(mode, '+') != NULL)
{
- fp->zstd.output.size = ZSTD_CStreamOutSize();
- fp->zstd.output.dst = pg_malloc0(fp->zstd.output.size);
- fp->zstd.cstream = ZstdCStreamParams(compression);
+ fp->u.zstd.output.size = ZSTD_CStreamOutSize();
+ fp->u.zstd.output.dst = pg_malloc0(fp->u.zstd.output.size);
+ fp->u.zstd.cstream = ZstdCStreamParams(compression);
}
else if (strchr(mode, 'r'))
{
- fp->zstd.input.src = pg_malloc0(ZSTD_DStreamInSize());
- fp->zstd.dstream = ZSTD_createDStream();
- if (fp->zstd.dstream == NULL)
+ fp->u.zstd.input.src = pg_malloc0(ZSTD_DStreamInSize());
+ fp->u.zstd.dstream = ZSTD_createDStream();
+ if (fp->u.zstd.dstream == NULL)
fatal("could not initialize compression library");
} // XXX else: bad mode
return fp;
#endif
case COMPR_ALG_NONE:
- fp->uncompressedfp = fopen(path, mode);
- if (fp->uncompressedfp == NULL)
+ fp->u.fp = fopen(path, mode);
+ if (fp->u.fp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -830,6 +836,8 @@ cfdopen(int fd, const char *mode, Compress *compression)
{
cfp *fp = pg_malloc0(sizeof(cfp));
+ fp->alg = compression->alg;
+
switch (compression->alg)
{
#ifdef HAVE_LIBZ
@@ -841,15 +849,15 @@ cfdopen(int fd, const char *mode, Compress *compression)
snprintf(mode_compression, sizeof(mode_compression), "%s%d",
mode, compression->level);
- fp->compressedfp = gzdopen(fd, mode_compression);
+ fp->u.gzfp = gzdopen(fd, mode_compression);
}
else
{
/* don't specify a level, just use the zlib default */
- fp->compressedfp = gzdopen(fd, mode);
+ fp->u.gzfp = gzdopen(fd, mode);
}
- if (fp->compressedfp == NULL)
+ if (fp->u.gzfp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -859,8 +867,8 @@ cfdopen(int fd, const char *mode, Compress *compression)
#ifdef HAVE_LIBZSTD
case COMPR_ALG_ZSTD:
- fp->zstd.fp = fdopen(fd, mode);
- if (fp->zstd.fp == NULL)
+ fp->u.zstd.fp = fdopen(fd, mode);
+ if (fp->u.zstd.fp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -868,23 +876,23 @@ cfdopen(int fd, const char *mode, Compress *compression)
else if (mode[0] == 'w' || mode[0] == 'a' ||
strchr(mode, '+') != NULL)
{
- fp->zstd.output.size = ZSTD_CStreamOutSize();
- fp->zstd.output.dst = pg_malloc0(fp->zstd.output.size);
- fp->zstd.cstream = ZstdCStreamParams(compression);
+ fp->u.zstd.output.size = ZSTD_CStreamOutSize();
+ fp->u.zstd.output.dst = pg_malloc0(fp->u.zstd.output.size);
+ fp->u.zstd.cstream = ZstdCStreamParams(compression);
}
else if (strchr(mode, 'r'))
{
- fp->zstd.input.src = pg_malloc0(ZSTD_DStreamInSize());
- fp->zstd.dstream = ZSTD_createDStream();
- if (fp->zstd.dstream == NULL)
+ fp->u.zstd.input.src = pg_malloc0(ZSTD_DStreamInSize());
+ fp->u.zstd.dstream = ZSTD_createDStream();
+ if (fp->u.zstd.dstream == NULL)
fatal("could not initialize compression library");
} // XXX else: bad mode
return fp;
#endif
case COMPR_ALG_NONE:
- fp->uncompressedfp = fdopen(fd, mode);
- if (fp->uncompressedfp == NULL)
+ fp->u.fp = fdopen(fd, mode);
+ if (fp->u.fp == NULL)
{
free_keep_errno(fp);
fp = NULL;
@@ -908,13 +916,13 @@ cfread(void *ptr, int size, cfp *fp)
return 0;
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
+ if (fp->alg == COMPR_ALG_LIBZ)
{
- ret = gzread(fp->compressedfp, ptr, size);
- if (ret != size && !gzeof(fp->compressedfp))
+ ret = gzread(fp->u.gzfp, ptr, size);
+ if (ret != size && !gzeof(fp->u.gzfp))
{
int errnum;
- const char *errmsg = gzerror(fp->compressedfp, &errnum);
+ const char *errmsg = gzerror(fp->u.gzfp, &errnum);
fatal("could not read from input file: %s",
errnum == Z_ERRNO ? strerror(errno) : errmsg);
@@ -924,10 +932,10 @@ cfread(void *ptr, int size, cfp *fp)
#endif
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
+ if (fp->alg == COMPR_ALG_ZSTD)
{
- ZSTD_outBuffer *output = &fp->zstd.output;
- ZSTD_inBuffer *input = &fp->zstd.input;
+ ZSTD_outBuffer *output = &fp->u.zstd.output;
+ ZSTD_inBuffer *input = &fp->u.zstd.input;
size_t input_size = ZSTD_DStreamInSize();
/* input_size is the allocated size */
size_t res, cnt;
@@ -953,7 +961,7 @@ cfread(void *ptr, int size, cfp *fp)
/* read compressed data if we must produce more input */
if (input->pos == input->size)
{
- cnt = fread(unconstify(void *, input->src), 1, input_size, fp->zstd.fp);
+ cnt = fread(unconstify(void *, input->src), 1, input_size, fp->u.zstd.fp);
input->size = cnt;
/* If we have no input to consume, we're done */
@@ -968,7 +976,7 @@ cfread(void *ptr, int size, cfp *fp)
for ( ; input->pos < input->size; )
{
/* decompress */
- res = ZSTD_decompressStream(fp->zstd.dstream, output, input);
+ res = ZSTD_decompressStream(fp->u.zstd.dstream, output, input);
if (res == 0)
break; /* End of frame */
if (output->pos == output->size)
@@ -985,9 +993,9 @@ cfread(void *ptr, int size, cfp *fp)
}
#endif
- ret = fread(ptr, 1, size, fp->uncompressedfp);
- if (ret != size && !feof(fp->uncompressedfp))
- READ_ERROR_EXIT(fp->uncompressedfp);
+ ret = fread(ptr, 1, size, fp->u.fp);
+ if (ret != size && !feof(fp->u.fp))
+ READ_ERROR_EXIT(fp->u.fp);
return ret;
}
@@ -995,16 +1003,16 @@ int
cfwrite(const void *ptr, int size, cfp *fp)
{
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
- return gzwrite(fp->compressedfp, ptr, size);
+ if (fp->alg == COMPR_ALG_LIBZ)
+ return gzwrite(fp->u.gzfp, ptr, size);
#endif
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
+ if (fp->alg == COMPR_ALG_ZSTD)
{
size_t res, cnt;
- ZSTD_outBuffer *output = &fp->zstd.output;
- ZSTD_inBuffer *input = &fp->zstd.input;
+ ZSTD_outBuffer *output = &fp->u.zstd.output;
+ ZSTD_inBuffer *input = &fp->u.zstd.input;
input->src = ptr;
input->size = size;
@@ -1014,11 +1022,11 @@ cfwrite(const void *ptr, int size, cfp *fp)
while (input->pos != input->size)
{
output->pos = 0;
- res = ZSTD_compressStream2(fp->zstd.cstream, output, input, ZSTD_e_continue);
+ res = ZSTD_compressStream2(fp->u.zstd.cstream, output, input, ZSTD_e_continue);
if (ZSTD_isError(res))
fatal("could not compress data: %s", ZSTD_getErrorName(res));
- cnt = fwrite(output->dst, 1, output->pos, fp->zstd.fp);
+ cnt = fwrite(output->dst, 1, output->pos, fp->u.zstd.fp);
if (cnt != output->pos)
fatal("could not write data: %s", strerror(errno));
}
@@ -1027,7 +1035,7 @@ cfwrite(const void *ptr, int size, cfp *fp)
}
#endif
- return fwrite(ptr, 1, size, fp->uncompressedfp);
+ return fwrite(ptr, 1, size, fp->u.fp);
}
int
@@ -1036,12 +1044,12 @@ cfgetc(cfp *fp)
int ret;
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
+ if (fp->alg == COMPR_ALG_LIBZ)
{
- ret = gzgetc(fp->compressedfp);
+ ret = gzgetc(fp->u.gzfp);
if (ret == EOF)
{
- if (!gzeof(fp->compressedfp))
+ if (!gzeof(fp->u.gzfp))
fatal("could not read from input file: %s", strerror(errno));
else
fatal("could not read from input file: end of file");
@@ -1051,11 +1059,11 @@ cfgetc(cfp *fp)
#endif
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
+ if (fp->alg == COMPR_ALG_ZSTD)
{
if (cfread(&ret, 1, fp) != 1)
{
- if (feof(fp->zstd.fp))
+ if (feof(fp->u.zstd.fp))
fatal("could not read from input file: end of file");
else
fatal("could not read from input file: %s", strerror(errno));
@@ -1064,9 +1072,9 @@ cfgetc(cfp *fp)
}
#endif
- ret = fgetc(fp->uncompressedfp);
+ ret = fgetc(fp->u.fp);
if (ret == EOF)
- READ_ERROR_EXIT(fp->uncompressedfp);
+ READ_ERROR_EXIT(fp->u.fp);
return ret;
}
@@ -1074,11 +1082,12 @@ char *
cfgets(cfp *fp, char *buf, int len)
{
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
- return gzgets(fp->compressedfp, buf, len);
+ if (fp->alg == COMPR_ALG_LIBZ)
+ return gzgets(fp->u.gzfp, buf, len);
#endif
+
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
+ if (fp->alg == COMPR_ALG_ZSTD)
{
/*
* Read one byte at a time until newline or EOF.
@@ -1102,7 +1111,7 @@ cfgets(cfp *fp, char *buf, int len)
}
#endif
- return fgets(buf, len, fp->uncompressedfp);
+ return fgets(buf, len, fp->u.fp);
}
/* Close the given compressed or uncompressed stream; return 0 on success. */
@@ -1117,54 +1126,54 @@ cfclose(cfp *fp)
return EOF;
}
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
+ if (fp->alg == COMPR_ALG_LIBZ)
{
- result = gzclose(fp->compressedfp);
- fp->compressedfp = NULL;
+ result = gzclose(fp->u.gzfp);
+ fp->u.gzfp = NULL;
return result;
}
#endif
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
+ if (fp->alg == COMPR_ALG_ZSTD)
{
- ZSTD_outBuffer *output = &fp->zstd.output;
- ZSTD_inBuffer *input = &fp->zstd.input;
+ ZSTD_outBuffer *output = &fp->u.zstd.output;
+ ZSTD_inBuffer *input = &fp->u.zstd.input;
size_t res, cnt;
- if (fp->zstd.cstream)
+ if (fp->u.zstd.cstream)
{
for (;;)
{
output->pos = 0;
- res = ZSTD_compressStream2(fp->zstd.cstream, output, input, ZSTD_e_end);
+ res = ZSTD_compressStream2(fp->u.zstd.cstream, output, input, ZSTD_e_end);
if (ZSTD_isError(res))
fatal("could not compress data: %s", ZSTD_getErrorName(res));
- cnt = fwrite(output->dst, 1, output->pos, fp->zstd.fp);
+ cnt = fwrite(output->dst, 1, output->pos, fp->u.zstd.fp);
if (cnt != output->pos)
fatal("could not write data: %s", strerror(errno));
if (res == 0)
break;
}
- ZSTD_freeCStream(fp->zstd.cstream);
- pg_free(fp->zstd.output.dst);
+ ZSTD_freeCStream(fp->u.zstd.cstream);
+ pg_free(fp->u.zstd.output.dst);
}
- if (fp->zstd.dstream)
+ if (fp->u.zstd.dstream)
{
- ZSTD_freeDStream(fp->zstd.dstream);
- pg_free(unconstify(void *, fp->zstd.input.src));
+ ZSTD_freeDStream(fp->u.zstd.dstream);
+ pg_free(unconstify(void *, fp->u.zstd.input.src));
}
- result = fclose(fp->zstd.fp);
- fp->zstd.fp = NULL;
+ result = fclose(fp->u.zstd.fp);
+ fp->u.zstd.fp = NULL;
return result;
}
#endif
- result = fclose(fp->uncompressedfp);
- fp->uncompressedfp = NULL;
+ result = fclose(fp->u.fp);
+ fp->u.fp = NULL;
free_keep_errno(fp);
return result;
}
@@ -1173,25 +1182,26 @@ int
cfeof(cfp *fp)
{
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
- return gzeof(fp->compressedfp);
+ if (fp->alg == COMPR_ALG_LIBZ)
+ return gzeof(fp->u.gzfp);
#endif
#ifdef HAVE_LIBZSTD
- if (fp->zstd.fp)
- return feof(fp->zstd.fp);
+ if (fp->alg == COMPR_ALG_ZSTD)
+ return feof(fp->u.zstd.fp);
#endif
- return feof(fp->uncompressedfp);
+
+ return feof(fp->u.fp);
}
const char *
get_cfp_error(cfp *fp)
{
#ifdef HAVE_LIBZ
- if (fp->compressedfp)
+ if (fp->alg == COMPR_ALG_LIBZ)
{
int errnum;
- const char *errmsg = gzerror(fp->compressedfp, &errnum);
+ const char *errmsg = gzerror(fp->u.gzfp, &errnum);
if (errnum != Z_ERRNO)
return errmsg;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7c2f7a9ca3..5e009e5854 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -397,7 +397,7 @@ parse_compression(const char *optarg, Compress *compress)
const int default_compress_level[] = {
0, /* COMPR_ALG_NONE */
Z_DEFAULT_COMPRESSION, /* COMPR_ALG_ZLIB */
- 0, // XXX: ZSTD_CLEVEL_DEFAULT, /* COMPR_ALG_ZSTD */
+ 0, // #ifdef LIBZSTD ZSTD_CLEVEL_DEFAULT, /* COMPR_ALG_ZSTD */
};
compress->level = default_compress_level[compress->alg];
--
2.17.0
--GRPZ8SYKNexpdSJ7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0009-Move-zlib-into-the-union.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption?
@ 2023-06-29 17:50 Nikolay Samokhvalov <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Nikolay Samokhvalov @ 2023-06-29 17:50 UTC (permalink / raw)
To: pgsql-hackers
Hi!
(posting this to -hackers rather than to -docs since it seems a deeper
problem than just adjusting the docs)
I recently observed a case with standby corruption after upgrading pg12 to
pg14, which was presented in the form of XX001 errors on the new cluster's
standby nodes. e.g.:
ERROR: missing chunk number 0 for toast value 3228893903 in
pg_toast_79504413
Comparing the content of the data directory and checking files with md5sum,
I noticed that some files for some TOAST index have different content on
new standby nodes compared to the new primary – and interesting was the
fact all standbys had the same content. Just different compared to the
primary.
We used the "rsync --size-only" snippet from the docs
https://www.postgresql.org/docs/current/pgupgrade.html to upgrade standbys.
With "--size-only", 1 GiB files for tables and indexes obviously cannot be
reliably synchronized. In our case, we perform additional steps involving
logical replication, advancing primary to certain LSN position -- and
during that, we keep standbys down. This explains the increased corruption
risks. But I think these risks are present for those who just follow the
steps in the docs as is, and probably some fixes or improvements are needed
here.
The main question: why do we consider "rsync --size-only" as reliable in
the general case? May standby corruption happen if we use follow steps from
https://www.postgresql.org/docs/current/pgupgrade.html?
Considering several general situations:
1. For streaming replication:
a. if we shut down the primary first, based on the code in walsender.c
defining how shutdown even is handled, replicas should receive all the
changes?
b. if shut down standbys first (might be preferred if we run cluster
under Patroni control, to avoid unnecessary failover), then some changes
from the primary won't be received by standbys – and we do have standby
corruption risks
2. For replication based on WAL shipping, I don't think we can guarantee
that all changes are propagated to standbys.
The docs also have this:
> 9. Prepare for standby server upgrades
> If you are upgrading standby servers using methods outlined in section
Step 11, verify that the old standby servers are caught up by running
pg_controldata against the old primary and standby clusters. Verify that
the “Latest checkpoint location” values match in all clusters. (There will
be a mismatch if old standby servers were shut down before the old primary
or if the old standby servers are still running.) Also, make sure wal_level
is not set to minimal in the postgresql.conf file on the new primary
cluster.
– admitting that there might be mismatch. But if there is mismatch, rsync
--size-only is not going to help synchronize properly, right?
I was thinking about how to improve here, some ideas:
- "rsync --checksum" doesn't seem to be a good idea, it's, unfortunately,
very, very slow, though it would be the most reliable approach (but since
it's slow, I guess it's not worth even mentioning, crossing this out)
- we could remove "--size-only" and rely on default rsync behavior –
checking size and modification time; but how reliable would it be in
general case?
- make the step verifying “Latest checkpoint location” *after* shutting
down all nodes as mandatory, with instructions on how to avoid mismatch:
e.g., shut down primary first, disabling automated failover software, if
any, then run pg_controldata on standbys while they are running, and on
primary while it's already shut down (probably, different instructions are
needed for WAL shipping and streaming cases)
- probably, we should always run "rsync --checksum" for pg_wal
- I think, it's time to provide a snippet to run "rsync" in multiple
threads. A lot of installations today have many vCPUs and fast SSDs, and
running single-threaded rsync seems to be very slow (especially if we do
need to move away from "--size-only"). If it makes sense, I could come up
with some patch proposal for the docs
- it's probably time to implement support for standby upgrade in
pg_upgrade itself, finding some way to take care of standbys and moving
away from the need to run rsync or to rebuild standby nodes? Although, this
is just a raw idea without a proper proposal yet.
Does this make sense or I'm missing something and the current docs describe
a reliable process? (As I said, we have deviated from the process, to
involve logical replication, so I'm not 100% sure I'm right suspecting the
original procedure in having standby corruption risks.)
Thanks,
Nikolay Samokhvalov
Founder, Postgres.ai
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption?
@ 2023-06-29 18:38 Robert Haas <[email protected]>
parent: Nikolay Samokhvalov <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Robert Haas @ 2023-06-29 18:38 UTC (permalink / raw)
To: Nikolay Samokhvalov <[email protected]>; +Cc: pgsql-hackers
On Thu, Jun 29, 2023 at 1:50 PM Nikolay Samokhvalov <[email protected]> wrote:
> Does this make sense or I'm missing something and the current docs describe a reliable process? (As I said, we have deviated from the process, to involve logical replication, so I'm not 100% sure I'm right suspecting the original procedure in having standby corruption risks.)
I'm very suspicious about this section of the documentation. It
doesn't explain why --size-only is used or why --no-inc-recursive is
used.
> > 9. Prepare for standby server upgrades
> > If you are upgrading standby servers using methods outlined in section Step 11, verify that the old standby servers are caught up by running pg_controldata against the old primary and standby clusters. Verify that the “Latest checkpoint location” values match in all clusters. (There will be a mismatch if old standby servers were shut down before the old primary or if the old standby servers are still running.) Also, make sure wal_level is not set to minimal in the postgresql.conf file on the new primary cluster.
>
> – admitting that there might be mismatch. But if there is mismatch, rsync --size-only is not going to help synchronize properly, right?
I think the idea is that you shouldn't use the procedure in this case.
But honestly I don't think it's probably a good idea to use this
procedure at all. It's not clear enough under what circumstances, if
any, it's safe to use, and there's not really any way to know if
you've done it correctly. You couldn't pay me enough to recommend this
procedure to anyone.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption?
@ 2023-06-30 17:41 Bruce Momjian <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Bruce Momjian @ 2023-06-30 17:41 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Nikolay Samokhvalov <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
On Thu, Jun 29, 2023 at 02:38:58PM -0400, Robert Haas wrote:
> On Thu, Jun 29, 2023 at 1:50 PM Nikolay Samokhvalov <[email protected]> wrote:
> > Does this make sense or I'm missing something and the current docs describe a reliable process? (As I said, we have deviated from the process, to involve logical replication, so I'm not 100% sure I'm right suspecting the original procedure in having standby corruption risks.)
>
> I'm very suspicious about this section of the documentation. It
> doesn't explain why --size-only is used or why --no-inc-recursive is
> used.
I think --size-only was chosen only because it is the minimal comparison
option.
> > > 9. Prepare for standby server upgrades
> > > If you are upgrading standby servers using methods outlined in section Step 11, verify that the old standby servers are caught up by running pg_controldata against the old primary and standby clusters. Verify that the “Latest checkpoint location” values match in all clusters. (There will be a mismatch if old standby servers were shut down before the old primary or if the old standby servers are still running.) Also, make sure wal_level is not set to minimal in the postgresql.conf file on the new primary cluster.
> >
> > – admitting that there might be mismatch. But if there is mismatch, rsync --size-only is not going to help synchronize properly, right?
>
> I think the idea is that you shouldn't use the procedure in this case.
> But honestly I don't think it's probably a good idea to use this
> procedure at all. It's not clear enough under what circumstances, if
> any, it's safe to use, and there's not really any way to know if
> you've done it correctly. You couldn't pay me enough to recommend this
> procedure to anyone.
I think it would be good to revisit all the steps outlined in that
procedure and check which ones are still valid or need adjusting. It is
very possible the original steps have bugs or that new Postgres features
added since the steps were created don't work with these steps. I think
we need to bring Stephen Frost into the discussion, so I have CC'ed him.
Frankly, I didn't think the documented procedure would work either, but
people say it does, so it is in the docs. I do think it is overdue for
a re-analysis.
--
Bruce Momjian <[email protected]> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-06-30 17:41 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-21 06:11 [PATCH 08/20] union{} with a CompressionAlgorithm alg Justin Pryzby <[email protected]>
2023-06-29 17:50 pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption? Nikolay Samokhvalov <[email protected]>
2023-06-29 18:38 ` Re: pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption? Robert Haas <[email protected]>
2023-06-30 17:41 ` Re: pg_upgrade instructions involving "rsync --size-only" might lead to standby corruption? Bruce Momjian <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox