| From: | "Hayato Kuroda (Fujitsu)" <kuroda(dot)hayato(at)fujitsu(dot)com> |
|---|---|
| To: | 'Andrey Silitskiy' <a(dot)silitskiy(at)postgrespro(dot)ru>, Alexander Korotkov <aekorotkov(at)gmail(dot)com> |
| Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, "Takamichi Osumi (Fujitsu)" <osumi(dot)takamichi(at)fujitsu(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "sawada(dot)mshk(at)gmail(dot)com" <sawada(dot)mshk(at)gmail(dot)com>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz>, "peter(dot)eisentraut(at)enterprisedb(dot)com" <peter(dot)eisentraut(at)enterprisedb(dot)com>, "dilipbalaut(at)gmail(dot)com" <dilipbalaut(at)gmail(dot)com>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>, "amit(dot)kapila16(at)gmail(dot)com" <amit(dot)kapila16(at)gmail(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Greg Sabino Mullane <htamfids(at)gmail(dot)com>, Vitaly Davydov <v(dot)davydov(at)postgrespro(dot)ru> |
| Subject: | RE: Exit walsender before confirming remote flush in logical replication |
| Date: | 2026-01-15 09:48:35 |
| Message-ID: | TY7PR01MB1455448E1FDAD6E381C692EF8F58CA@TY7PR01MB14554.jpnprd01.prod.outlook.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Dear Andrey,
Sorry for being late I've missed the thread. I read the patch but the test may
contain some issues.
1.
Please update meson.build, otherwise it won't be run by Meson/Ninja builds.
2.
Missing Copyright, please add below at the top of the file.
```
# Copyright (c) 2026, PostgreSQL Global Development Group
```
3.
```
# run concurrent transaction on publisher and commit
$out = $publisher->safe_psql('postgres', 'BEGIN; INSERT INTO pub_test VALUES (0); COMMIT;');
ok($out eq "", "Concurrent transaction was committed on publisher");
```
IIUC, safe_psql() ensures the command is succeeded. Isn't it enough to use below?
```
$publisher->safe_psql('postgres', 'INSERT INTO pub_test VALUES (0);');
```
4.
```
# test publisher shutdown
ok_with_timeout(5, sub { $publisher->stop('fast') },
"Successfull fast shutdown of server with empty output buffers");
```
I think we can just use stop() because it internally runs `pg_ctl stop` and
that command waits till the wait is finished by default. I feel it is dangerous
to determine timeout to 5sec because the test can work on very poor environment.
This means `run_with_timeout`, `ok_with_timeout` are not needed anymore.
5.
```
$subscriber->wait_for_subscription_sync($publisher, 'sub_all');
```
I think this at line 114 should be wait_for_catchup().
6.
```
# generate big amount of wal records for locked table
$out = $publisher->safe_psql('postgres', 'BEGIN; INSERT INTO pub_test SELECT i from generate_series(1, 20000) s(i); COMMIT;');
ok($out eq "", "Inserts into locked table successfully generated");
```
Same thing can be said as 3. This means `$out` may not be needed anymore.
Also, not sure, how can we ensure the buffer is full here? Also, even if we have
the way to check, the size may be quite platform depending.
I think it may be better to test both streaming and logical replication instead
of testing empty/full output buffer. Thought?
(In this case the test can be put in recovery/ subdir)
Best regards,
Hayato Kuroda
FUJITSU LIMITED
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ashutosh Bapat | 2026-01-15 09:51:56 | Re: Refactor replication origin state reset helpers |
| Previous Message | Hannu Krosing | 2026-01-15 09:46:36 | Re: Patch: dumping tables data in multiple chunks in pg_dump |