Re: Small miscellaneous fixes
On Sat, Nov 26, 2022 at 11:30:07AM -0300, Ranier Vilela wrote: > Thank you Michael, for taking care of it. (As of 1e31484, after finishing the tests I wanted.) -- Michael signature.asc Description: PGP signature
Re: Small miscellaneous fixes
Em sex., 25 de nov. de 2022 às 22:21, Michael Paquier escreveu: > On Fri, Nov 25, 2022 at 01:15:40PM +0100, Peter Eisentraut wrote: > > Is this something you want to follow up on, since you were involved in > that > > patch? Is the redundant assignment simply to be deleted, or do you want > to > > check the original patch again for context? > > Most of the changes of this thread have been applied as of c42cd05c. > Remains the SIGALRM business with sig_atomic_t, and I wanted to check > that by myself first. > Thank you Michael, for taking care of it. regards, Ranier Vilela
Re: Small miscellaneous fixes
On Fri, Nov 25, 2022 at 01:15:40PM +0100, Peter Eisentraut wrote: > Is this something you want to follow up on, since you were involved in that > patch? Is the redundant assignment simply to be deleted, or do you want to > check the original patch again for context? Most of the changes of this thread have been applied as of c42cd05c. Remains the SIGALRM business with sig_atomic_t, and I wanted to check that by myself first. -- Michael signature.asc Description: PGP signature
Re: Small miscellaneous fixes
On 04.10.22 06:18, Michael Paquier wrote: On Mon, Oct 03, 2022 at 08:05:57AM -0300, Ranier Vilela wrote: Em seg., 3 de out. de 2022 às 05:01, Masahiko Sawada escreveu: On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela wrote: 1. Avoid useless reassigning var _logsegno (src/backend/access/transam/xlog.c) Commit 7d70809 left a little oversight. XLByteToPrevSeg and XLByteToSeg are macros, and both assign _logsegno. So, the first assignment is lost and is useless. Right, I have missed this one. We do that now in build_backup_content() when building the contents of the backup history file. Is this something you want to follow up on, since you were involved in that patch? Is the redundant assignment simply to be deleted, or do you want to check the original patch again for context?
Re: Small miscellaneous fixes
Em qua., 16 de nov. de 2022 às 03:59, Michael Paquier escreveu: > On Tue, Oct 04, 2022 at 08:23:16AM -0300, Ranier Vilela wrote: > > Both are correct, I missed the pqsignal calls. > > > > Attached patch to change this. > > The change for pgbench is missing and this is only changing > pg_test_fsync. Switching to sig_atomic_t would be fine on non-WIN32 > as these are used in signal handlers, but are we sure that this is > fine on WIN32 for pg_test_fsync where we rely on a separate thread to > control the timing of the alarm? > Well I tested here in Windows 10 64 bits with sig_atomic_t alarm_triggered and works fine. ctrl + c breaks the exe. Windows 10 64bits SSD 256GB For curiosity, this is the test results: 5 seconds per test O_DIRECT supported on this platform for open_datasync and open_sync. Compare file sync methods using one 8kB write: (in wal_sync_method preference order, except fdatasync is Linux's default) open_datasync ctrl + c C:\postgres_debug\bin>pg_test_fsync 5 seconds per test O_DIRECT supported on this platform for open_datasync and open_sync. Compare file sync methods using one 8kB write: (in wal_sync_method preference order, except fdatasync is Linux's default) open_datasync ctrl + c C:\postgres_debug\bin>pg_test_fsync 5 seconds per test O_DIRECT supported on this platform for open_datasync and open_sync. Compare file sync methods using one 8kB write: (in wal_sync_method preference order, except fdatasync is Linux's default) open_datasync 9495,720 ops/sec 105 usecs/op fdatasync 444,174 ops/sec2251 usecs/op fsync 398,487 ops/sec2509 usecs/op fsync_writethrough 342,018 ops/sec2924 usecs/op open_sync n/a Compare file sync methods using two 8kB writes: (in wal_sync_method preference order, except fdatasync is Linux's default) open_datasync 4719,825 ops/sec 212 usecs/op fdatasync 442,138 ops/sec2262 usecs/op fsync 401,163 ops/sec2493 usecs/op fsync_writethrough 397,198 ops/sec2518 usecs/op open_sync n/a Compare open_sync with different write sizes: (This is designed to compare the cost of writing 16kB in different write open_sync sizes.) 1 * 16kB open_sync write n/a 2 * 8kB open_sync writes n/a 4 * 4kB open_sync writes n/a 8 * 2kB open_sync writes n/a 16 * 1kB open_sync writes n/a Test if fsync on non-write file descriptor is honored: (If the times are similar, fsync() can sync data written on a different descriptor.) write, fsync, close 77,808 ops/sec 12852 usecs/op write, close, fsync 77,469 ops/sec 12908 usecs/op Non-sync'ed 8kB writes: write139789,685 ops/sec 7 usecs/op regards, Ranier Vilela
Re: Small miscellaneous fixes
On Tue, Oct 04, 2022 at 08:23:16AM -0300, Ranier Vilela wrote: > Both are correct, I missed the pqsignal calls. > > Attached patch to change this. The change for pgbench is missing and this is only changing pg_test_fsync. Switching to sig_atomic_t would be fine on non-WIN32 as these are used in signal handlers, but are we sure that this is fine on WIN32 for pg_test_fsync where we rely on a separate thread to control the timing of the alarm? -- Michael signature.asc Description: PGP signature
Re: Small miscellaneous fixes
Em ter., 4 de out. de 2022 às 01:18, Michael Paquier escreveu: > On Mon, Oct 03, 2022 at 08:05:57AM -0300, Ranier Vilela wrote: > > Em seg., 3 de out. de 2022 às 05:01, Masahiko Sawada < > sawada.m...@gmail.com> > > escreveu: > >> On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela > wrote: > >>> 1. Avoid useless reassigning var _logsegno > >> (src/backend/access/transam/xlog.c) > >>> Commit 7d70809 left a little oversight. > >>> XLByteToPrevSeg and XLByteToSeg are macros, and both assign _logsegno. > >>> So, the first assignment is lost and is useless. > > Right, I have missed this one. We do that now in > build_backup_content() when building the contents of the backup > history file. > > >>> 4. Fix declaration volatile signal var (src/bin/pgbench/pgbench.c) > >>> Like how to commit 5ac9e86, this is a similar case. > >> > >> The same is true also for alarm_triggered in pg_test_fsync.c? > >> > > I don't think so. > > If I understand the problem correctly, the failure can occur with true > > signals, provided by the OS > > In the case at hand, it seems to me more like an internal form of signal, > > that is, simulated. > > So bool works fine. > > I am not following your reasoning here. Why does it matter to change > one but not the other? Both are used with SIGALRM, it seems. > Both are correct, I missed the pqsignal calls. Attached patch to change this. > The other three seem fine, so fixed. > Thanks Michael for the commit. regards, Ranier Vilela fix_declaration_volatile_signal_pg_test_fsync.patch Description: Binary data
Re: Small miscellaneous fixes
On Mon, Oct 03, 2022 at 08:05:57AM -0300, Ranier Vilela wrote: > Em seg., 3 de out. de 2022 às 05:01, Masahiko Sawada > escreveu: >> On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela wrote: >>> 1. Avoid useless reassigning var _logsegno >> (src/backend/access/transam/xlog.c) >>> Commit 7d70809 left a little oversight. >>> XLByteToPrevSeg and XLByteToSeg are macros, and both assign _logsegno. >>> So, the first assignment is lost and is useless. Right, I have missed this one. We do that now in build_backup_content() when building the contents of the backup history file. >>> 4. Fix declaration volatile signal var (src/bin/pgbench/pgbench.c) >>> Like how to commit 5ac9e86, this is a similar case. >> >> The same is true also for alarm_triggered in pg_test_fsync.c? >> > I don't think so. > If I understand the problem correctly, the failure can occur with true > signals, provided by the OS > In the case at hand, it seems to me more like an internal form of signal, > that is, simulated. > So bool works fine. I am not following your reasoning here. Why does it matter to change one but not the other? Both are used with SIGALRM, it seems. The other three seem fine, so fixed. -- Michael signature.asc Description: PGP signature
Re: Small miscellaneous fixes
Em seg., 3 de out. de 2022 às 05:01, Masahiko Sawada escreveu: > On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela wrote: > > > > Hi. > > > > There are assorted fixes to the head branch. > > > > 1. Avoid useless reassigning var _logsegno > (src/backend/access/transam/xlog.c) > > Commit 7d70809 left a little oversight. > > XLByteToPrevSeg and XLByteToSeg are macros, and both assign _logsegno. > > So, the first assignment is lost and is useless. > > > > 2. Avoid retesting log_min_duration (src/backend/commands/analyze.c) > > The log_min_duration has already been tested before and the second test > > can be safely removed. > > > > 3. Avoid useless var declaration record (src/backend/utils/misc/guc.c) > > The var record is never really used. > > Three changes look good to me. > Hi, thanks for reviewing this. > > > > > 4. Fix declaration volatile signal var (src/bin/pgbench/pgbench.c) > > Like how to commit 5ac9e86, this is a similar case. > > The same is true also for alarm_triggered in pg_test_fsync.c? > I don't think so. If I understand the problem correctly, the failure can occur with true signals, provided by the OS In the case at hand, it seems to me more like an internal form of signal, that is, simulated. So bool works fine. CF entry created: https://commitfest.postgresql.org/40/3925/ regards, Ranier Vilela
Re: Small miscellaneous fixes
On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela wrote: > > Hi. > > There are assorted fixes to the head branch. > > 1. Avoid useless reassigning var _logsegno (src/backend/access/transam/xlog.c) > Commit 7d70809 left a little oversight. > XLByteToPrevSeg and XLByteToSeg are macros, and both assign _logsegno. > So, the first assignment is lost and is useless. > > 2. Avoid retesting log_min_duration (src/backend/commands/analyze.c) > The log_min_duration has already been tested before and the second test > can be safely removed. > > 3. Avoid useless var declaration record (src/backend/utils/misc/guc.c) > The var record is never really used. Three changes look good to me. > > 4. Fix declaration volatile signal var (src/bin/pgbench/pgbench.c) > Like how to commit 5ac9e86, this is a similar case. The same is true also for alarm_triggered in pg_test_fsync.c? Regards, -- Masahiko Sawada PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
