Обсуждение: pgsql: Exclude unlogged tables from base backups
Exclude unlogged tables from base backups Exclude unlogged tables from base backup entirely except init fork which marks created unlogged table. The next question is do not backup temp table but it's a story for separate patch. Author: David Steele Review by: Adam Brightwell, Masahiko Sawada Discussion: https://www.postgresql.org/message-id/flat/04791bab-cb04-ba43-e9c0-664a4c1ffb2c@pgmasters.net Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/8694cc96b52a967a49725f32be7aa77fd3b6ac25 Modified Files -------------- doc/src/sgml/protocol.sgml | 6 +++ src/backend/replication/basebackup.c | 64 ++++++++++++++++++++++++++++ src/backend/storage/file/reinit.c | 4 +- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 42 +++++++++++++++++- src/include/storage/reinit.h | 2 + 5 files changed, 113 insertions(+), 5 deletions(-)
Teodor Sigaev <teodor@sigaev.ru> writes:
> Exclude unlogged tables from base backups
Buildfarm member skink (valgrind) has reported this during its last couple
of runs:
2018-03-24 03:18:23.409 UTC [17302] 010_pg_basebackup.pl LOG: received replication command: BASE_BACKUP LABEL
'pg_basebackupbase backup' NOWAIT TABLESPACE_MAP
==17302== VALGRINDERROR-BEGIN
==17302== Conditional jump or move depends on uninitialised value(s)
==17302== at 0x4C2FD88: strlen (vg_replace_strmem.c:458)
==17302== by 0x6FBA3AB: vfprintf (vfprintf.c:1643)
==17302== by 0x6FE19AF: vsnprintf (vsnprintf.c:114)
==17302== by 0x6FC0F6E: snprintf (snprintf.c:33)
==17302== by 0x4A3C97: sendDir (basebackup.c:1060)
==17302== by 0x4A40C6: sendDir (basebackup.c:1221)
==17302== by 0x4A52B2: sendTablespace (basebackup.c:937)
==17302== by 0x4A5740: perform_base_backup (basebackup.c:313)
==17302== by 0x4A635C: SendBaseBackup (basebackup.c:710)
==17302== by 0x49FCF2: exec_replication_command (walsender.c:1520)
==17302== by 0x4F61CE: PostgresMain (postgres.c:4143)
==17302== by 0x4712ED: BackendRun (postmaster.c:4409)
==17302== Uninitialised value was created by a stack allocation
==17302== at 0x4A38EA: sendDir (basebackup.c:957)
==17302==
==17302== VALGRINDERROR-END
2018-03-24 03:18:26.127 UTC [17302] 010_pg_basebackup.pl DEBUG: contents of directory "pg_serial" excluded from backup
I might be wrong to blame that on this patch, but nothing else has
touched basebackup.c lately.
regards, tom lane
On 3/25/18 2:16 PM, Tom Lane wrote: > Teodor Sigaev <teodor@sigaev.ru> writes: >> Exclude unlogged tables from base backups > > Buildfarm member skink (valgrind) has reported this during its last couple > of runs: > > 2018-03-24 03:18:23.409 UTC [17302] 010_pg_basebackup.pl LOG: received replication command: BASE_BACKUP LABEL 'pg_basebackupbase backup' NOWAIT TABLESPACE_MAP > ==17302== VALGRINDERROR-BEGIN > ==17302== Conditional jump or move depends on uninitialised value(s) > ==17302== at 0x4C2FD88: strlen (vg_replace_strmem.c:458) > ==17302== by 0x6FBA3AB: vfprintf (vfprintf.c:1643) > ==17302== by 0x6FE19AF: vsnprintf (vsnprintf.c:114) > ==17302== by 0x6FC0F6E: snprintf (snprintf.c:33) > ==17302== by 0x4A3C97: sendDir (basebackup.c:1060) > ==17302== by 0x4A40C6: sendDir (basebackup.c:1221) > ==17302== by 0x4A52B2: sendTablespace (basebackup.c:937) > ==17302== by 0x4A5740: perform_base_backup (basebackup.c:313) > ==17302== by 0x4A635C: SendBaseBackup (basebackup.c:710) > ==17302== by 0x49FCF2: exec_replication_command (walsender.c:1520) > ==17302== by 0x4F61CE: PostgresMain (postgres.c:4143) > ==17302== by 0x4712ED: BackendRun (postmaster.c:4409) > ==17302== Uninitialised value was created by a stack allocation > ==17302== at 0x4A38EA: sendDir (basebackup.c:957) > ==17302== > ==17302== VALGRINDERROR-END > 2018-03-24 03:18:26.127 UTC [17302] 010_pg_basebackup.pl DEBUG: contents of directory "pg_serial" excluded from backup > > I might be wrong to blame that on this patch, but nothing else has > touched basebackup.c lately. Sure looks like it deserves the blame -- the error is occurring right in the new code. I think skink is using large values for rel oids and that has exposed a bug. The strncpy doesn't zero terminate the string if the oid has the max number of characters. At least, I was able to reproduce under those circumstances. The attached should fix it. Regards, -- -David david@pgmasters.net
Вложения
I wrote:
> I might be wrong to blame that on this patch, but nothing else has
> touched basebackup.c lately.
Ah, I see the problem, I think. Fixed.
regards, tom lane
David Steele <david@pgmasters.net> writes:
> On 3/25/18 2:16 PM, Tom Lane wrote:
>> Buildfarm member skink (valgrind) has reported this during its last couple
>> of runs:
> I think skink is using large values for rel oids and that has exposed a
> bug. The strncpy doesn't zero terminate the string if the oid has the
> max number of characters. At least, I was able to reproduce under those
> circumstances.
Actually, that code didn't guarantee zero termination under *any*
circumstances; it only happened to work if the stack contained
zeroes to start with.
> The attached should fix it.
Found this in my inbox right after pushing a fix. I did it slightly
differently, emulating the later rather than earlier calls in reinit.c.
The earlier ones memset the whole target field because they're concerned
about being able to hash it, but we don't need that here, just zero
termination.
regards, tom lane
On 3/25/18 3:22 PM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 3/25/18 2:16 PM, Tom Lane wrote: >>> Buildfarm member skink (valgrind) has reported this during its last couple >>> of runs: > >> I think skink is using large values for rel oids and that has exposed a >> bug. The strncpy doesn't zero terminate the string if the oid has the >> max number of characters. At least, I was able to reproduce under those >> circumstances. > > Actually, that code didn't guarantee zero termination under *any* > circumstances; it only happened to work if the stack contained > zeroes to start with. Interesting. strncpy() says it will pad the destination with NULLs when src is less than the size provided. Perhaps some compilers don't honor that? >> The attached should fix it. > > Found this in my inbox right after pushing a fix. I did it slightly > differently, emulating the later rather than earlier calls in reinit.c. > The earlier ones memset the whole target field because they're concerned > about being able to hash it, but we don't need that here, just zero > termination. Yeah, that's the way I would normally do it, but when I searched reinit.c the first few hits did memset() so I went with that. Thanks for taking care of it. -- -David david@pgmasters.net
David Steele <david@pgmasters.net> writes:
> On 3/25/18 3:22 PM, Tom Lane wrote:
>> Actually, that code didn't guarantee zero termination under *any*
>> circumstances; it only happened to work if the stack contained
>> zeroes to start with.
> Interesting. strncpy() says it will pad the destination with NULLs when
> src is less than the size provided. Perhaps some compilers don't honor
> that?
Yeah, but the "size provided" was the number of characters to be copied
from the source string, not the size of the destination buffer. So
strncpy didn't think it needed to add any nulls. There's a reason why
that function is widely disliked --- it's hard to use it in a safe way.
regards, tom lane
On 3/25/18 3:54 PM, Tom Lane wrote: > David Steele <david@pgmasters.net> writes: >> On 3/25/18 3:22 PM, Tom Lane wrote: >>> Actually, that code didn't guarantee zero termination under *any* >>> circumstances; it only happened to work if the stack contained >>> zeroes to start with. > >> Interesting. strncpy() says it will pad the destination with NULLs when >> src is less than the size provided. Perhaps some compilers don't honor >> that? > > Yeah, but the "size provided" was the number of characters to be copied > from the source string, not the size of the destination buffer. So > strncpy didn't think it needed to add any nulls. There's a reason why > that function is widely disliked --- it's hard to use it in a safe way. Whoops, how right you are. I'm generally passing destination buffer size in these cases and totally misread what this was doing. -- -David david@pgmasters.net