Horton

Thread

Commits

  1. Mark two signal flags as sig_atomic_t in pgbench and pg_test_fsync

  2. Cleanup useless assignments and checks

  3. Mark sigint_interrupt_enabled as sig_atomic_t

  4. Refactor creation of backup_label and backup history files

  1. Small miscellaneous fixes

    Ranier Vilela <ranier.vf@gmail.com> — 2022-09-30T00:08:02Z

    Hi.

There are assorted fixes to the head branch.

1. Avoid useless reassigning var _logsegno
(src/backend/access/transam/xlog.c)
Commit 7d70809
<https://github.com/postgres/postgres/commit/7d708093b7400327658a30d1aa1d5e284d37622c>
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
<https://github.com/postgres/postgres/commit/5ac9e869191148741539e626b84ba7e77dc71670>,
this is a similar case.

regards,
Ranier Vilela

  2. Re: Small miscellaneous fixes

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-10-03T08:01:16Z

    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

  3. Re: Small miscellaneous fixes

    Ranier Vilela <ranier.vf@gmail.com> — 2022-10-03T11:05:57Z

    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.

CF entry created:
https://commitfest.postgresql.org/40/3925/

regards,
Ranier Vilela

  4. Re: Small miscellaneous fixes

    Michael Paquier <michael@paquier.xyz> — 2022-10-04T04:18:34Z

    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

  5. Re: Small miscellaneous fixes

    Ranier Vilela <ranier.vf@gmail.com> — 2022-10-04T11:23:16Z

    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

  6. Re: Small miscellaneous fixes

    Michael Paquier <michael@paquier.xyz> — 2022-11-16T06:59:33Z

    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

  7. Re: Small miscellaneous fixes

    Ranier Vilela <ranier.vf@gmail.com> — 2022-11-16T17:56:11Z

    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)
        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

regards,
Ranier Vilela

  8. Re: Small miscellaneous fixes

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-25T12:15:40Z

    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?

  9. Re: Small miscellaneous fixes

    Michael Paquier <michael@paquier.xyz> — 2022-11-26T01:21:18Z

    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

  10. Re: Small miscellaneous fixes

    Ranier Vilela <ranier.vf@gmail.com> — 2022-11-26T14:30:07Z

    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

  11. Re: Small miscellaneous fixes

    Michael Paquier <michael@paquier.xyz> — 2022-11-27T05:13:47Z

    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