Small miscellaneous fixes

Started by Ranier Vilelaover 3 years ago11 messages

ranier.vf@gmail.com

over 3 years ago

4 attachment(s)

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

Attachments:

avoid_useless_reassign_lgosegno.patchapplication/octet-stream; name=avoid_useless_reassign_lgosegno.patchDownload

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 8e15256db8..27085b15a8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8733,7 +8733,6 @@ do_pg_backup_stop(BackupState *state, bool waitforarchive)
 		 */
 		RequestXLogSwitch(false);
 
-		XLByteToPrevSeg(state->stoppoint, _logSegNo, wal_segment_size);
 		state->stoptime = (pg_time_t) time(NULL);
 
 		/*

avoid_useless_retesting_log_min_duration.patchapplication/octet-stream; name=avoid_useless_retesting_log_min_duration.patchDownload

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index a7966fff83..ff1354812b 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -360,8 +360,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 		}
 
 		pg_rusage_init(&ru0);
-		if (params->log_min_duration >= 0)
-			starttime = GetCurrentTimestamp();
+		starttime = GetCurrentTimestamp();
 	}
 
 	/*

avoid_useless_var_record.patchapplication/octet-stream; name=avoid_useless_var_record.patchDownload

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ff082fc3d9..0135766035 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5262,12 +5262,10 @@ read_nondefault_variables(void)
 
 	for (;;)
 	{
-		struct config_generic *record;
-
 		if ((varname = read_string_with_null(fp)) == NULL)
 			break;
 
-		if ((record = find_option(varname, true, false, FATAL)) == NULL)
+		if (find_option(varname, true, false, FATAL) == NULL)
 			elog(FATAL, "failed to locate variable \"%s\" in exec config params file", varname);
 
 		if ((varvalue = read_string_with_null(fp)) == NULL)

fix_declaration_volatile_signal_var.patchapplication/octet-stream; name=fix_declaration_volatile_signal_var.patchDownload

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index aa1a3541fe..14867a4bd1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -310,7 +310,7 @@ const char *progname;
 
 #define WSEP '@'				/* weight separator */
 
-volatile bool timer_exceeded = false;	/* flag from signal handler */
+volatile sig_atomic_t timer_exceeded = false;	/* flag from signal handler */
 
 /*
  * We don't want to allocate variables one by one; for efficiency, add a

Masahiko Sawada

sawada.mshk@gmail.com

over 3 years ago

In reply to: Ranier Vilela (#1)

Re: Small miscellaneous fixes

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

Ranier Vilela

ranier.vf@gmail.com

over 3 years ago

In reply to: Masahiko Sawada (#2)

Re: Small miscellaneous fixes

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

Michael Paquier

michael@paquier.xyz

over 3 years ago

In reply to: Ranier Vilela (#3)

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

Ranier Vilela

ranier.vf@gmail.com

over 3 years ago

In reply to: Michael Paquier (#4)

1 attachment(s)

Re: Small miscellaneous fixes

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

Attachments:

fix_declaration_volatile_signal_pg_test_fsync.patchapplication/octet-stream; name=fix_declaration_volatile_signal_pg_test_fsync.patchDownload

diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index 5f8cbb75ff..3d5e8f30ab 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -68,7 +68,7 @@ static char full_buf[DEFAULT_XLOG_SEG_SIZE],
 		   *filename = FSYNC_FILENAME;
 static struct timeval start_t,
 			stop_t;
-static bool alarm_triggered = false;
+static sig_atomic_t alarm_triggered = false;
 
 
 static void handle_args(int argc, char *argv[]);

Michael Paquier

michael@paquier.xyz

about 3 years ago

In reply to: Ranier Vilela (#5)

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

Ranier Vilela

ranier.vf@gmail.com

about 3 years ago

In reply to: Michael Paquier (#6)

Re: Small miscellaneous fixes

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

Peter Eisentraut

peter.eisentraut@enterprisedb.com

about 3 years ago

In reply to: Michael Paquier (#4)

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

Michael Paquier

michael@paquier.xyz

about 3 years ago

In reply to: Peter Eisentraut (#8)

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

#10

Ranier Vilela

ranier.vf@gmail.com

about 3 years ago

In reply to: Michael Paquier (#9)

Re: Small miscellaneous fixes

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

Michael Paquier

michael@paquier.xyz

about 3 years ago

In reply to: Ranier Vilela (#10)

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