Thread: Small miscellaneous fixes
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.
4. Fix declaration volatile signal var (src/bin/pgbench/pgbench.c)
Like how to commit 5ac9e86, this is a similar case.
regards,
Ranier Vilela
Attachment
On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela <ranier.vf@gmail.com> 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
Em seg., 3 de out. de 2022 às 05:01, Masahiko Sawada <sawada.mshk@gmail.com> escreveu:
On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela <ranier.vf@gmail.com> 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.
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:
regards,
Ranier Vilela
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.mshk@gmail.com> > escreveu: >> On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela <ranier.vf@gmail.com> 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
Attachment
Em ter., 4 de out. de 2022 às 01:18, Michael Paquier <michael@paquier.xyz> 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.mshk@gmail.com>
> escreveu:
>> On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela <ranier.vf@gmail.com> 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
Attachment
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
Attachment
Em qua., 16 de nov. de 2022 às 03:59, Michael Paquier <michael@paquier.xyz> 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)
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/sec 2251 usecs/op
fsync 398,487 ops/sec 2509 usecs/op
fsync_writethrough 342,018 ops/sec 2924 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/sec 2262 usecs/op
fsync 401,163 ops/sec 2493 usecs/op
fsync_writethrough 397,198 ops/sec 2518 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:
write 139789,685 ops/sec 7 usecs/op
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/sec 2251 usecs/op
fsync 398,487 ops/sec 2509 usecs/op
fsync_writethrough 342,018 ops/sec 2924 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/sec 2262 usecs/op
fsync 401,163 ops/sec 2493 usecs/op
fsync_writethrough 397,198 ops/sec 2518 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:
write 139789,685 ops/sec 7 usecs/op
regards,
Ranier Vilela
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 <sawada.mshk@gmail.com> >> escreveu: >>> On Fri, Sep 30, 2022 at 9:08 AM Ranier Vilela <ranier.vf@gmail.com> 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?
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
Attachment
Em sex., 25 de nov. de 2022 às 22:21, Michael Paquier <michael@paquier.xyz> 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
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