Re: [PATCH] Fix minor issues in astreamer_zstd.c
| От | zengman |
|---|---|
| Тема | Re: [PATCH] Fix minor issues in astreamer_zstd.c |
| Дата | |
| Msg-id | tencent_12935413436FD4730C456DA8@qq.com обсуждение исходный текст |
| Ответ на | Re: [PATCH] Fix minor issues in astreamer_zstd.c (Michael Paquier <michael@paquier.xyz>) |
| Список | pgsql-hackers |
> > Could you demonstrate one or more examples when using these APIs
> > proving that in some cases the current code can be a problem while the
> > "fixed" code improves the situation, then extract test cases to be
> > able to cover our future tracks? This would take the shape of one or
> > more regression tests to demonstrate individual problems. If the
> > three code paths touched here prove to be problematic, we would need
> > three cases in total. One other possibility would be to use a set of
> > asserts to make sure that nobody uses these APIs in ways we don't
> > expect them to.
>
> See for example 3369a3b49b0b as one reference, that has fixed a bug in
> the same area of the code.
Hi Mr. Michael
Thank you for pointing this out. I'd like to admit my mistake first. I was actually just looking at the code in
`astreamer_zstd.c`and noticed that `astreamer_zstd_decompressor_finalize` checked `zstd_outBuf.pos` but didn't use it,
whichpuzzled me.
```
if (mystreamer->zstd_outBuf.pos > 0)
astreamer_content(mystreamer->base.bbs_next, NULL,
mystreamer->base.bbs_buffer.data,
mystreamer->base.bbs_buffer.maxlen,
ASTREAMER_UNKNOWN);
```
Then I referred to the rest of the code in that file and made the corresponding adjustments. Before that, when I tested
itmanually, there was probably an error in the operation, and the size difference was about 3kb, so I thought that was
theproblem.
However, after several more tests, I found no difference.
Before code modification:
```
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ pg_basebackup --pgdata ~/zstd_backup --format tar --compress
zstd:1--no-sync --manifest-checksums sha256
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ pg_verifybackup --exit-on-error ~/zstd_backup -n
backup successfully verified
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ ls -al ~/zstd_backup
total 19500
drwx------ 2 postgres postgres 4096 Jan 11 17:11 .
drwx------ 28 postgres root 4096 Jan 11 17:11 ..
-rw------- 1 postgres postgres 177649 Jan 11 17:11 backup_manifest
-rw------- 1 postgres postgres 2996548 Jan 11 17:11 base.tar.zst
-rw------- 1 postgres postgres 16778752 Jan 11 17:11 pg_wal.tar
```
After modifying the code:
```
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ pg_basebackup --pgdata ~/zstd_backup2 --format tar --compress
zstd:1--no-sync --manifest-checksums sha256
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ pg_verifybackup --exit-on-error ~/zstd_backup2 -n
backup successfully verified
postgres@zxm-VMware-Virtual-Platform:~/code/postgres$ ls ~/zstd_backup2 -al
total 19500
drwx------ 2 postgres postgres 4096 Jan 11 17:13 .
drwx------ 29 postgres root 4096 Jan 11 17:13 ..
-rw------- 1 postgres postgres 177649 Jan 11 17:13 backup_manifest
-rw------- 1 postgres postgres 2996546 Jan 11 17:13 base.tar.zst
-rw------- 1 postgres postgres 16778752 Jan 11 17:13 pg_wal.tar
```
So from a code perspective, this might be worth modifying, but not modifying it doesn't seem to cause any real harm.
Sorryto bother you all.
В списке pgsql-hackers по дате отправления: