On Thu, Dec 22, 2022 at 7:34 AM Thomas Munro <thomas.mu...@gmail.com> wrote:
>
> On Wed, Dec 14, 2022 at 5:48 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> > 0001 -- David's palloc_aligned() patch
> > https://commitfest.postgresql.org/41/3999/
> > 0002 -- I/O-align almost all buffers used for I/O
> > 0003 -- Add the GUCs
> > 0004 -- Throwaway hack to make cfbot turn the GUCs on
>
> David pushed the first as commit 439f6175, so here is a rebase of the
> rest. I also fixed a couple of thinkos in the handling of systems
> where we don't know how to do direct I/O. In one place I had #ifdef
> PG_O_DIRECT, but that's always defined, it's just that it's 0 on
> Solaris and OpenBSD, and the check to reject the GUC wasn't quite
> right on such systems.
Thanks. I have some comments on
v3-0002-Add-io_direct-setting-developer-only.patch:
1. I think we don't need to overwrite the io_direct_string in
check_io_direct so that show_io_direct can be avoided.
2. check_io_direct can leak the flags memory - when io_direct is not
supported or for an invalid list syntax or an invalid option is
specified.
I have addressed my review comments as a delta patch on top of v3-0002
and added it here as v1-0001-Review-comments-io_direct-GUC.txt.
Some comments on the tests added:
1. Is there a way to know if Direct IO for WAL and data has been
picked up programmatically? IOW, can we know if the OS page cache is
bypassed? I know an external extension pgfincore which can help here,
but nothing in the core exists AFAICS.
+is('10000', $node->safe_psql('postgres', 'select count(*) from t1'),
"read back from shared");
+is('10000', $node->safe_psql('postgres', 'select * from t2count'),
"read back from local");
+$node->stop('immediate');
2. Can we combine these two append_conf to a single statement?
+$node->append_conf('io_direct', 'data,wal,wal_init');
+$node->append_conf('shared_buffers', '64kB'); # tiny to force I/O
3. A nitpick: Can we split these queries multi-line instead of in a single line?
+$node->safe_psql('postgres', 'begin; create temporary table t2 as
select 1 as i from generate_series(1, 10000); update t2 set i = i;
insert into t2count select count(*) from t2; commit;');
4. I don't think we need to stop the node before the test ends, no?
+$node->stop;
+
+done_testing();
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From b3eed3d6fc849b9e16fbace1f37d401424f81ab0 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com>
Date: Wed, 25 Jan 2023 07:18:11 +0000
Subject: [PATCH v1] Review comments io_direct GUC
---
src/backend/storage/file/fd.c | 57 ++++++++++++-----------------
src/backend/utils/misc/guc_tables.c | 4 +-
src/include/utils/guc_hooks.h | 1 -
3 files changed, 25 insertions(+), 37 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index eb83de4fb9..329acc2ffd 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3749,7 +3749,7 @@ data_sync_elevel(int elevel)
bool
check_io_direct(char **newval, void **extra, GucSource source)
{
- int *flags = guc_malloc(ERROR, sizeof(*flags));
+ int flags;
#if PG_O_DIRECT == 0
if (strcmp(*newval, "") != 0)
@@ -3757,38 +3757,51 @@ check_io_direct(char **newval, void **extra, GucSource
source)
GUC_check_errdetail("io_direct is not supported on this
platform.");
return false;
}
- *flags = 0;
+ flags = 0;
#else
- List *list;
+ List *elemlist;
ListCell *l;
+ char *rawstring;
- if (!SplitGUCList(*newval, ',', &list))
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ if (!SplitGUCList(rawstring, ',', &elemlist))
{
GUC_check_errdetail("invalid list syntax in parameter \"%s\"",
"io_direct");
+ pfree(rawstring);
+ list_free(elemlist);
return false;
}
- *flags = 0;
- foreach (l, list)
+ flags = 0;
+ foreach (l, elemlist)
{
char *item = (char *) lfirst(l);
if (pg_strcasecmp(item, "data") == 0)
- *flags |= IO_DIRECT_DATA;
+ flags |= IO_DIRECT_DATA;
else if (pg_strcasecmp(item, "wal") == 0)
- *flags |= IO_DIRECT_WAL;
+ flags |= IO_DIRECT_WAL;
else if (pg_strcasecmp(item, "wal_init") == 0)
- *flags |= IO_DIRECT_WAL_INIT;
+ flags |= IO_DIRECT_WAL_INIT;
else
{
GUC_check_errdetail("invalid option \"%s\"", item);
+ pfree(rawstring);
+ list_free(elemlist);
return false;
}
}
+
+ pfree(rawstring);
+ list_free(elemlist);
#endif
- *extra = flags;
+ /* Save the flags in *extra, for use by assign_io_direct */
+ *extra = guc_malloc(ERROR, sizeof(int));
+ *((int *) *extra) = flags;
return true;
}
@@ -3800,27 +3813,3 @@ assign_io_direct(const char *newval, void *extra)
io_direct_flags = *flags;
}
-
-extern const char *
-show_io_direct(void)
-{
- static char result[80];
-
- result[0] = 0;
- if (io_direct_flags & IO_DIRECT_DATA)
- strcat(result, "data");
- if (io_direct_flags & IO_DIRECT_WAL)
- {
- if (result[0])
- strcat(result, ", ");
- strcat(result, "wal");
- }
- if (io_direct_flags & IO_DIRECT_WAL_INIT)
- {
- if (result[0])
- strcat(result, ", ");
- strcat(result, "wal_init");
- }
-
- return result;
-}
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 9410493ae7..25b7e87abb 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4529,11 +4529,11 @@ struct config_string ConfigureNamesString[] =
{"io_direct", PGC_POSTMASTER, DEVELOPER_OPTIONS,
gettext_noop("Use direct I/O for file access."),
NULL,
- GUC_NOT_IN_SAMPLE
+ GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE
},
&io_direct_string,
"",
- check_io_direct, assign_io_direct, show_io_direct
+ check_io_direct, assign_io_direct, NULL
},
/* End-of-list marker */
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index e656a16a40..b3b5148185 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -156,6 +156,5 @@ extern void assign_wal_consistency_checking(const char
*newval, void *extra);
extern void assign_xlog_sync_method(int new_sync_method, void *extra);
extern bool check_io_direct(char **newval, void **extra, GucSource source);
extern void assign_io_direct(const char *newval, void *extra);
-extern const char *show_io_direct(void);
#endif /* GUC_HOOKS_H */
--
2.34.1
