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 по дате отправления: