Обсуждение: pgsql: Use XLOG_BLCKSZ in pg_test_fsync, rather than our own define, bu
Use XLOG_BLCKSZ in pg_test_fsync, rather than our own define, but verify it is 8k as expected. Branch ------ master Details ------- http://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=9dd7933937a076ce7573944b8d1d42e618163440 Modified Files -------------- contrib/pg_test_fsync/pg_test_fsync.c | 24 +++++++++++++----------- 1 files changed, 13 insertions(+), 11 deletions(-)
Bruce Momjian <bruce@momjian.us> writes:
> Use XLOG_BLCKSZ in pg_test_fsync, rather than our own define, but verify
> it is 8k as expected.
-#define WRITE_SIZE (8 * 1024) /* 8k */
+#if XLOG_BLCKSZ != 8 * 1024 /* 8k */
+#error Unknown block size for test.
+#endif
This seems like a pretty awful idea. Aren't you aware that XLOG_BLCKSZ
is settable from a configure option? You just broke the ability to
build the tree with a non-default configuration setting.
If you aren't willing to deal with a variable value for the block size,
please revert this patch.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Use XLOG_BLCKSZ in pg_test_fsync, rather than our own define, but verify > > it is 8k as expected. > > -#define WRITE_SIZE (8 * 1024) /* 8k */ > +#if XLOG_BLCKSZ != 8 * 1024 /* 8k */ > +#error Unknown block size for test. > +#endif > > This seems like a pretty awful idea. Aren't you aware that XLOG_BLCKSZ > is settable from a configure option? You just broke the ability to > build the tree with a non-default configuration setting. > > If you aren't willing to deal with a variable value for the block size, > please revert this patch. The problem is that I have hard-coded 8k into various text strings and I didn't want to make that variable. How should it behave if they are using a non-8k wal buffer size? Should it still use 8k or not? I figured throwing an error would at least alert them to the mismatch. Good point on the build problem --- I had not thought of that. Throwing an error when running makes more sense, but let's figure out what it should do. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> If you aren't willing to deal with a variable value for the block size,
>> please revert this patch.
> The problem is that I have hard-coded 8k into various text strings and I
> didn't want to make that variable. How should it behave if they are
> using a non-8k wal buffer size? Should it still use 8k or not? I
> figured throwing an error would at least alert them to the mismatch.
Well, as I said, if you aren't willing to put effort into that point,
just revert the patch. Making the program refuse to do anything doesn't
help *anyone*. Stats taken using a fixed 8K blocksize are better than
no stats at all.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Tom Lane wrote: > >> If you aren't willing to deal with a variable value for the block size, > >> please revert this patch. > > > The problem is that I have hard-coded 8k into various text strings and I > > didn't want to make that variable. How should it behave if they are > > using a non-8k wal buffer size? Should it still use 8k or not? I > > figured throwing an error would at least alert them to the mismatch. > > Well, as I said, if you aren't willing to put effort into that point, > just revert the patch. Making the program refuse to do anything doesn't > help *anyone*. Stats taken using a fixed 8K blocksize are better than > no stats at all. Sure I am willing to fix it. Should I have it always use the value of XLOG_BLCKSZ for its tests, and adjust the output text accordingly? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Bruce Momjian <bruce@momjian.us> writes:
> Sure I am willing to fix it. Should I have it always use the value of
> XLOG_BLCKSZ for its tests, and adjust the output text accordingly?
Makes sense to me.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > Sure I am willing to fix it. Should I have it always use the value of > > XLOG_BLCKSZ for its tests, and adjust the output text accordingly? > > Makes sense to me. OK, done with attached patch. I also cleaned up the open_sync size test output. The patch was easy. Thanks for the ideas. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_test_fsync/pg_test_fsync.c b/contrib/pg_test_fsync/pg_test_fsync.c index 5e6406a..a3a49a1 100644 *** a/contrib/pg_test_fsync/pg_test_fsync.c --- b/contrib/pg_test_fsync/pg_test_fsync.c *************** *** 20,28 **** */ #define FSYNC_FILENAME "./pg_test_fsync.out" ! #if XLOG_BLCKSZ != 8 * 1024 /* 8k */ ! #error Unknown block size for test. ! #endif #define LABEL_FORMAT " %-32s" #define NA_FORMAT LABEL_FORMAT "%18s" --- 20,26 ---- */ #define FSYNC_FILENAME "./pg_test_fsync.out" ! #define XLOG_BLCKSZ_K (XLOG_BLCKSZ / 1024) #define LABEL_FORMAT " %-32s" #define NA_FORMAT LABEL_FORMAT "%18s" *************** main(int argc, char *argv[]) *** 61,70 **** test_open(); ! /* Test using 1 8k write */ test_sync(1); ! /* Test using 2 8k writes */ test_sync(2); test_open_syncs(); --- 59,68 ---- test_open(); ! /* Test using 1 XLOG_BLCKSZ write */ test_sync(1); ! /* Test using 2 XLOG_BLCKSZ writes */ test_sync(2); test_open_syncs(); *************** test_sync(int writes_per_op) *** 177,185 **** bool fs_warning = false; if (writes_per_op == 1) ! printf("\nCompare file sync methods using one 8k write:\n"); else ! printf("\nCompare file sync methods using two 8k writes:\n"); printf("(in wal_sync_method preference order, except fdatasync\n"); printf("is Linux's default)\n"); --- 175,183 ---- bool fs_warning = false; if (writes_per_op == 1) ! printf("\nCompare file sync methods using one %dk write:\n", XLOG_BLCKSZ_K); else ! printf("\nCompare file sync methods using two %dk writes:\n", XLOG_BLCKSZ_K); printf("(in wal_sync_method preference order, except fdatasync\n"); printf("is Linux's default)\n"); *************** test_open_syncs(void) *** 396,406 **** printf("(This is designed to compare the cost of writing 16k\n"); printf("in different write open_sync sizes.)\n"); ! test_open_sync(" 1 16k open_sync write", 16); ! test_open_sync(" 2 8k open_sync writes", 8); ! test_open_sync(" 4 4k open_sync writes", 4); ! test_open_sync(" 8 2k open_sync writes", 2); ! test_open_sync("16 1k open_sync writes", 1); } /* --- 394,404 ---- printf("(This is designed to compare the cost of writing 16k\n"); printf("in different write open_sync sizes.)\n"); ! test_open_sync("16k open_sync write", 16); ! test_open_sync(" 8k open_sync writes", 8); ! test_open_sync(" 4k open_sync writes", 4); ! test_open_sync(" 2k open_sync writes", 2); ! test_open_sync(" 1k open_sync writes", 1); } /* *************** test_non_sync(void) *** 519,525 **** /* * Test a simple write without fsync */ ! printf("\nNon-sync'ed 8k writes:\n"); printf(LABEL_FORMAT, "write"); fflush(stdout); --- 517,523 ---- /* * Test a simple write without fsync */ ! printf("\nNon-sync'ed %dk writes:\n", XLOG_BLCKSZ_K); printf(LABEL_FORMAT, "write"); fflush(stdout);