public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v9 1/8] pg_dump: make CLUSTER ON a separate dump object.. 4+ messages / 3 participants [nested] [flat]
* [PATCH v9 1/8] pg_dump: make CLUSTER ON a separate dump object.. @ 2020-11-26 20:37 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-11-26 20:37 UTC (permalink / raw) ..since it needs to be restored after any child indexes are restored *and attached*. The order needs to be: 1) restore child and parent index (order doesn't matter); 2) attach child index; 3) set cluster on child and parent index (order doesn't matter); --- src/bin/pg_dump/pg_dump.c | 86 ++++++++++++++++++++++++++-------- src/bin/pg_dump/pg_dump.h | 8 ++++ src/bin/pg_dump/pg_dump_sort.c | 8 ++++ 3 files changed, 82 insertions(+), 20 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index eb988d7eb4..e93d2eb828 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -208,6 +208,7 @@ static void dumpSequence(Archive *fout, const TableInfo *tbinfo); static void dumpSequenceData(Archive *fout, const TableDataInfo *tdinfo); static void dumpIndex(Archive *fout, const IndxInfo *indxinfo); static void dumpIndexAttach(Archive *fout, const IndexAttachInfo *attachinfo); +static void dumpIndexClusterOn(Archive *fout, const IndexClusterInfo *clusterinfo); static void dumpStatisticsExt(Archive *fout, const StatsExtInfo *statsextinfo); static void dumpConstraint(Archive *fout, const ConstraintInfo *coninfo); static void dumpTableConstraintComment(Archive *fout, const ConstraintInfo *coninfo); @@ -7092,6 +7093,11 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) i_inddependcollversions; int ntups; + int ncluster = 0; + IndexClusterInfo *clusterinfo; + clusterinfo = (IndexClusterInfo *) + pg_malloc0(numTables * sizeof(IndexClusterInfo)); + for (i = 0; i < numTables; i++) { TableInfo *tbinfo = &tblinfo[i]; @@ -7471,6 +7477,27 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables) /* Plain secondary index */ indxinfo[j].indexconstraint = 0; } + + /* Record each table's CLUSTERed index, if any */ + if (indxinfo[j].indisclustered) + { + IndxInfo *index = &indxinfo[j]; + IndexClusterInfo *cluster = &clusterinfo[ncluster]; + + cluster->dobj.objType = DO_INDEX_CLUSTER_ON; + cluster->dobj.catId.tableoid = 0; + cluster->dobj.catId.oid = 0; + AssignDumpId(&cluster->dobj); + cluster->dobj.name = pg_strdup(index->dobj.name); + cluster->dobj.namespace = index->indextable->dobj.namespace; + cluster->index = index; + cluster->indextable = &tblinfo[i]; + + /* The CLUSTER ON depends on its index.. */ + addObjectDependency(&cluster->dobj, index->dobj.dumpId); + + ncluster++; + } } PQclear(res); @@ -10323,6 +10350,9 @@ dumpDumpableObject(Archive *fout, const DumpableObject *dobj) case DO_SUBSCRIPTION: dumpSubscription(fout, (const SubscriptionInfo *) dobj); break; + case DO_INDEX_CLUSTER_ON: + dumpIndexClusterOn(fout, (IndexClusterInfo *) dobj); + break; case DO_PRE_DATA_BOUNDARY: case DO_POST_DATA_BOUNDARY: /* never dumped, nothing to do */ @@ -16543,6 +16573,41 @@ getAttrName(int attrnum, const TableInfo *tblInfo) return NULL; /* keep compiler quiet */ } +/* + * dumpIndexClusterOn + * record that the index is clustered. + */ +static void +dumpIndexClusterOn(Archive *fout, const IndexClusterInfo *clusterinfo) +{ + DumpOptions *dopt = fout->dopt; + TableInfo *tbinfo = clusterinfo->indextable; + char *qindxname; + PQExpBuffer q; + + if (dopt->dataOnly) + return; + + q = createPQExpBuffer(); + qindxname = pg_strdup(fmtId(clusterinfo->dobj.name)); + + /* index name is not qualified in this syntax */ + appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER ON %s;\n", + fmtQualifiedDumpable(tbinfo), qindxname); + + if (clusterinfo->dobj.dump & DUMP_COMPONENT_DEFINITION) + ArchiveEntry(fout, clusterinfo->dobj.catId, clusterinfo->dobj.dumpId, + ARCHIVE_OPTS(.tag = clusterinfo->dobj.name, + .namespace = tbinfo->dobj.namespace->dobj.name, + .owner = tbinfo->rolname, + .description = "INDEX CLUSTER ON", + .section = SECTION_POST_DATA, + .createStmt = q->data)); + + destroyPQExpBuffer(q); + free(qindxname); +} + /* * dumpIndex * write out to fout a user-defined index @@ -16597,16 +16662,6 @@ dumpIndex(Archive *fout, const IndxInfo *indxinfo) * similar code in dumpConstraint! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - qindxname); - } - /* * If the index has any statistics on some of its columns, generate * the associated ALTER INDEX queries. @@ -16933,16 +16988,6 @@ dumpConstraint(Archive *fout, const ConstraintInfo *coninfo) * similar code in dumpIndex! */ - /* If the index is clustered, we need to record that. */ - if (indxinfo->indisclustered) - { - appendPQExpBuffer(q, "\nALTER TABLE %s CLUSTER", - fmtQualifiedDumpable(tbinfo)); - /* index name is not qualified in this syntax */ - appendPQExpBuffer(q, " ON %s;\n", - fmtId(indxinfo->dobj.name)); - } - /* If the index defines identity, we need to record that. */ if (indxinfo->indisreplident) { @@ -18448,6 +18493,7 @@ addBoundaryDependencies(DumpableObject **dobjs, int numObjs, break; case DO_INDEX: case DO_INDEX_ATTACH: + case DO_INDEX_CLUSTER_ON: case DO_STATSEXT: case DO_REFRESH_MATVIEW: case DO_TRIGGER: diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index 0a2213fb06..627c8fbdab 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -54,6 +54,7 @@ typedef enum DO_ATTRDEF, DO_INDEX, DO_INDEX_ATTACH, + DO_INDEX_CLUSTER_ON, DO_STATSEXT, DO_RULE, DO_TRIGGER, @@ -386,6 +387,13 @@ typedef struct _indxInfo DumpId indexconstraint; } IndxInfo; +typedef struct _indexClusterInfo +{ + DumpableObject dobj; + TableInfo *indextable; /* link to table the index is for */ + IndxInfo *index; /* link to index itself */ +} IndexClusterInfo; + typedef struct _indexAttachInfo { DumpableObject dobj; diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 46461fb6a1..dd5b233196 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -75,6 +75,7 @@ enum dbObjectTypePriorities PRIO_CONSTRAINT, PRIO_INDEX, PRIO_INDEX_ATTACH, + PRIO_INDEX_CLUSTER_ON, PRIO_STATSEXT, PRIO_RULE, PRIO_TRIGGER, @@ -108,6 +109,7 @@ static const int dbObjectTypePriority[] = PRIO_ATTRDEF, /* DO_ATTRDEF */ PRIO_INDEX, /* DO_INDEX */ PRIO_INDEX_ATTACH, /* DO_INDEX_ATTACH */ + PRIO_INDEX_CLUSTER_ON, /* DO_INDEX_CLUSTER_ON */ PRIO_STATSEXT, /* DO_STATSEXT */ PRIO_RULE, /* DO_RULE */ PRIO_TRIGGER, /* DO_TRIGGER */ @@ -136,6 +138,7 @@ static const int dbObjectTypePriority[] = PRIO_PUBLICATION, /* DO_PUBLICATION */ PRIO_PUBLICATION_REL, /* DO_PUBLICATION_REL */ PRIO_SUBSCRIPTION /* DO_SUBSCRIPTION */ + }; StaticAssertDecl(lengthof(dbObjectTypePriority) == (DO_SUBSCRIPTION + 1), @@ -1348,6 +1351,11 @@ describeDumpableObject(DumpableObject *obj, char *buf, int bufsize) "INDEX ATTACH %s (ID %d)", obj->name, obj->dumpId); return; + case DO_INDEX_CLUSTER_ON: + snprintf(buf, bufsize, + "INDEX CLUSTER ON %s (ID %d)", + obj->name, obj->dumpId); + return; case DO_STATSEXT: snprintf(buf, bufsize, "STATISTICS %s (ID %d OID %u)", -- 2.17.0 --fmvA4kSBHQVZhkR6 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v9-0002-Implement-CLUSTER-of-partitioned-table.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: warn if GUC set to an invalid shared library @ 2023-07-06 20:15 Justin Pryzby <[email protected]> 2023-12-28 05:26 ` Re: warn if GUC set to an invalid shared library Shubham Khanna <[email protected]> 2024-01-07 13:27 ` Re: warn if GUC set to an invalid shared library Justin Pryzby <[email protected]> 0 siblings, 2 replies; 4+ messages in thread From: Justin Pryzby @ 2023-07-06 20:15 UTC (permalink / raw) To: [email protected]; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; Bharath Rupireddy <[email protected]>; Cary Huang <[email protected]>; Maciek Sakrejda <[email protected]> On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <[email protected]> wrote: > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > It caused no issue when I changed: > > > > > > > > > > /* Check that it's acceptable for the indicated parameter */ > > > > > if (!parse_and_validate_value(record, name, value, > > > > > - PGC_S_FILE, ERROR, > > > > > + PGC_S_TEST, ERROR, > > > > > &newval, &newextra)) > > > > > > > > > > I'm not sure where to go from here. > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > sure what a wider change would look like. I'm still hoping. > > PGC_S_TEST is a better fit, so my question is whether it's really that > > simple ? > > I've added the trivial change as 0001 and re-opened the patch (which ended > up in January's CF) > > If for some reason it's not really as simple as that, then 001 will > serve as a "straw-man patch" hoping to elicit discussion on that point. > From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001 > From: Justin Pryzby <[email protected]> > Date: Fri, 22 Jul 2022 15:52:11 -0500 > Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not > FILE > > WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE > > Since the value didn't come from a file. Or maybe we should have > another PGC_S_ value for this, or a flag for 'is a test'. > --- > src/backend/utils/misc/guc.c | 2 +- > src/include/utils/guc.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > index 6f21752b844..ae8810591d6 100644 > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) > > /* Check that it's acceptable for the indicated parameter */ > if (!parse_and_validate_value(record, name, value, > - PGC_S_FILE, ERROR, > + PGC_S_TEST, ERROR, > &newval, &newextra)) > ereport(ERROR, > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), This is rebased over my own patch to enable checks for REGRESSION_TEST_NAME_RESTRICTIONS. -- Justin Attachments: [text/x-diff] 0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not-FI.patch (1.6K, ../../ZKcg1w4FoYq%[email protected]/2-0001-WIP-test-GUCs-from-ALTER-SYSTEM-as-PGC_S_TEST-not-FI.patch) download | inline diff: From aa4c3ae08b3379bcd222e00aa896a40f811155a5 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <[email protected]> Date: Fri, 22 Jul 2022 15:52:11 -0500 Subject: [PATCH 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not FILE WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE Since the value didn't come from a file. Or maybe we should have another PGC_S_ value for this, or a flag for 'is a test'. --- src/backend/utils/misc/guc.c | 2 +- src/include/utils/guc.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 5308896c87f..dda54310a56 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4540,7 +4540,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) /* Check that it's acceptable for the indicated parameter */ if (!parse_and_validate_value(record, name, value, - PGC_S_FILE, ERROR, + PGC_S_TEST, ERROR, &newval, &newextra)) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index d5253c7ed23..bd45d40c106 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -119,6 +119,7 @@ typedef enum PGC_S_OVERRIDE, /* special case to forcibly set default */ PGC_S_INTERACTIVE, /* dividing line for error reporting */ PGC_S_TEST, /* test per-database or per-user setting */ + // PGC_S_TEST_FILE, /* test global cluster settings (ALTER SYSTEM) */ PGC_S_SESSION /* SET command */ } GucSource; -- 2.34.1 [text/x-diff] 0002-warn-when-setting-GUC-to-a-nonextant-library.patch (10.8K, ../../ZKcg1w4FoYq%[email protected]/3-0002-warn-when-setting-GUC-to-a-nonextant-library.patch) download | inline diff: From b05c738bddfca11d4d0510c497a121cc8fcb585a Mon Sep 17 00:00:00 2001 From: Justin Pryzby <[email protected]> Date: Mon, 13 Dec 2021 08:42:38 -0600 Subject: [PATCH 2/4] warn when setting GUC to a nonextant library Note that warnings shouldn't be issued during startup (only fatals): $ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab 2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio 2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries" --- src/backend/commands/variable.c | 95 +++++++++++++++++++ src/backend/utils/misc/guc_tables.c | 6 +- src/include/utils/guc_hooks.h | 7 ++ .../unsafe_tests/expected/rolenames.out | 19 ++++ .../modules/unsafe_tests/sql/rolenames.sql | 13 +++ .../worker_spi/expected/worker_spi.out | 2 + .../modules/worker_spi/sql/worker_spi.sql | 2 + src/test/regress/expected/rules.out | 8 ++ 8 files changed, 149 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index f0f2e076552..9272a0f0e5a 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -40,6 +40,7 @@ #include "utils/timestamp.h" #include "utils/tzparser.h" #include "utils/varlena.h" +#include <unistd.h> /* * DATESTYLE @@ -1210,3 +1211,97 @@ check_ssl(bool *newval, void **extra, GucSource source) #endif return true; } + + +/* + * See also load_libraries() and internal_load_library(). + */ +static bool +check_preload_libraries(char **newval, void **extra, GucSource source, + bool restricted) +{ + char *rawstring; + List *elemlist; + ListCell *l; + + /* nothing to do if empty */ + if (newval == NULL || *newval[0] == '\0') + return true; + + /* + * issue warnings only during an interactive SET, from ALTER + * ROLE/DATABASE/SYSTEM. + */ + if (source != PGC_S_TEST) + return true; + + /* Need a modifiable copy of string */ + rawstring = pstrdup(*newval); + + /* Parse string into list of filename paths */ + if (!SplitDirectoriesString(rawstring, ',', &elemlist)) + { + /* Should not happen ? */ + return false; + } + + foreach(l, elemlist) + { + /* Note that filename was already canonicalized */ + char *filename = (char *) lfirst(l); + char *expanded = NULL; + + /* If restricting, insert $libdir/plugins if not mentioned already */ + if (restricted && first_dir_separator(filename) == NULL) + { + expanded = psprintf("$libdir/plugins/%s", filename); + filename = expanded; + } + + /* + * stat()/access() only check that the library exists, not that it has + * the correct magic number or even a library. But error messages + * from dlopen() are not portable, so it'd be hard to report any + * problem other than "does not exist". + */ + if (access(filename, R_OK) == 0) + continue; + + if (source == PGC_S_FILE) + /* ALTER SYSTEM */ + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errdetail("The server will currently fail to start with this setting."), + errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.", + "postgresql.auto.conf")); + else + /* ALTER ROLE/DATABASE */ + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errdetail("New sessions will currently fail to connect with the new setting.")); + } + + list_free_deep(elemlist); + pfree(rawstring); + return true; +} + +bool +check_shared_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, true); +} + +bool +check_local_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, false); +} + +bool +check_session_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, true); +} diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 59ab630ae40..e4a80d87928 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -4101,7 +4101,7 @@ struct config_string ConfigureNamesString[] = }, &session_preload_libraries_string, "", - NULL, NULL, NULL + check_session_preload_libraries, NULL, NULL }, { @@ -4112,7 +4112,7 @@ struct config_string ConfigureNamesString[] = }, &shared_preload_libraries_string, "", - NULL, NULL, NULL + check_shared_preload_libraries, NULL, NULL }, { @@ -4123,7 +4123,7 @@ struct config_string ConfigureNamesString[] = }, &local_preload_libraries_string, "", - NULL, NULL, NULL + check_local_preload_libraries, NULL, NULL }, { diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 2ecb9fc0866..de030d750f1 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -160,4 +160,11 @@ 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 bool check_local_preload_libraries(char **newval, void **extra, + GucSource source); +extern bool check_session_preload_libraries(char **newval, void **extra, + GucSource source); +extern bool check_shared_preload_libraries(char **newval, void **extra, + GucSource source); + #endif /* GUC_HOOKS_H */ diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out index 61396b2a805..2dcd7cf9ec0 100644 --- a/src/test/modules/unsafe_tests/expected/rolenames.out +++ b/src/test/modules/unsafe_tests/expected/rolenames.out @@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION; ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK; REVOKE pg_read_all_settings FROM regress_role_haspriv; +-- test some warnings +ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist"; +WARNING: could not access file "$libdir/plugins/DoesNotExist" +DETAIL: New sessions will currently fail to connect with the new setting. +ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist"; +WARNING: could not access file "DoesNotExist" +DETAIL: New sessions will currently fail to connect with the new setting. +CREATE DATABASE regression_nosuch_db; +ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist"; +WARNING: could not access file "$libdir/plugins/DoesNotExist" +DETAIL: New sessions will currently fail to connect with the new setting. +ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist"; +WARNING: could not access file "DoesNotExist" +DETAIL: New sessions will currently fail to connect with the new setting. +DROP DATABASE regression_nosuch_db; +-- SET doesn't do anything, but should not warn, either +SET session_preload_libraries="DoesNotExist"; +SET SESSION session_preload_libraries="DoesNotExist"; +begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback; -- clean up \c DROP SCHEMA test_roles_schema; diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql index adac36536db..56fb52d4e08 100644 --- a/src/test/modules/unsafe_tests/sql/rolenames.sql +++ b/src/test/modules/unsafe_tests/sql/rolenames.sql @@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION; ROLLBACK; REVOKE pg_read_all_settings FROM regress_role_haspriv; +-- test some warnings +ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist"; +ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist"; +CREATE DATABASE regression_nosuch_db; +ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist"; +ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist"; +DROP DATABASE regression_nosuch_db; + +-- SET doesn't do anything, but should not warn, either +SET session_preload_libraries="DoesNotExist"; +SET SESSION session_preload_libraries="DoesNotExist"; +begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback; + -- clean up \c diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out index dc0a79bf759..5b275669bcf 100644 --- a/src/test/modules/worker_spi/expected/worker_spi.out +++ b/src/test/modules/worker_spi/expected/worker_spi.out @@ -28,6 +28,8 @@ SELECT pg_reload_conf(); t (1 row) +-- reconnect to avoid unstable test result due to asynchronous signal +\c -- wait until the worker has processed the tuple we just inserted DO $$ DECLARE diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql index 4683523b29d..4ed5370d456 100644 --- a/src/test/modules/worker_spi/sql/worker_spi.sql +++ b/src/test/modules/worker_spi/sql/worker_spi.sql @@ -18,6 +18,8 @@ END $$; INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1); SELECT pg_reload_conf(); +-- reconnect to avoid unstable test result due to asynchronous signal +\c -- wait until the worker has processed the tuple we just inserted DO $$ DECLARE diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 7fd81e6a7d0..299c9cb4d2d 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3453,6 +3453,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer SET datestyle to iso, mdy SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789' IMMUTABLE STRICT; +WARNING: could not access file "Mixed/Case" +DETAIL: New sessions will currently fail to connect with the new setting. +WARNING: could not access file "c:/'a"/path" +DETAIL: New sessions will currently fail to connect with the new setting. +WARNING: could not access file "" +DETAIL: New sessions will currently fail to connect with the new setting. +WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" +DETAIL: New sessions will currently fail to connect with the new setting. SELECT pg_get_functiondef('func_with_set_params()'::regprocedure); pg_get_functiondef -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -- 2.34.1 [text/x-diff] 0003-errcontext-if-server-fails-to-start-due-to-library-G.patch (4.5K, ../../ZKcg1w4FoYq%[email protected]/4-0003-errcontext-if-server-fails-to-start-due-to-library-G.patch) download | inline diff: From 4a37cfde4b6e84cc309e2e2c99d24b88dcb1ddaf Mon Sep 17 00:00:00 2001 From: Justin Pryzby <[email protected]> Date: Sat, 18 Dec 2021 22:51:01 -0600 Subject: [PATCH 3/4] errcontext if server fails to start due to library GUCs --- src/backend/utils/fmgr/dfmgr.c | 21 ++++++++++++++++----- src/backend/utils/init/miscinit.c | 2 +- src/include/fmgr.h | 1 + 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index b85d52c913c..9674760fc02 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -75,7 +75,7 @@ static DynamicFileList *file_tail = NULL; char *Dynamic_library_path; -static void *internal_load_library(const char *libname); +static void *internal_load_library(const char *libname, const char *gucname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); static bool file_exists(const char *name); @@ -113,7 +113,7 @@ load_external_function(const char *filename, const char *funcname, fullname = expand_dynamic_library_name(filename); /* Load the shared library, unless we already did */ - lib_handle = internal_load_library(fullname); + lib_handle = internal_load_library(fullname, NULL); /* Return handle if caller wants it */ if (filehandle) @@ -142,6 +142,15 @@ load_external_function(const char *filename, const char *funcname, */ void load_file(const char *filename, bool restricted) +{ + load_file_guc(filename, restricted, NULL); +} + +/* + * Also accepts a GUC arg, for error reports + */ +void +load_file_guc(const char *filename, bool restricted, const char *gucname) { char *fullname; @@ -153,7 +162,7 @@ load_file(const char *filename, bool restricted) fullname = expand_dynamic_library_name(filename); /* Load the shared library */ - (void) internal_load_library(fullname); + (void) internal_load_library(fullname, gucname); pfree(fullname); } @@ -172,6 +181,7 @@ lookup_external_function(void *filehandle, const char *funcname) /* * Load the specified dynamic-link library file, unless it already is * loaded. Return the pg_dl* handle for the file. + * gucname may be passed for error reports. * * Note: libname is expected to be an exact name for the library file. * @@ -181,7 +191,7 @@ lookup_external_function(void *filehandle, const char *funcname) * perhaps other things that are definitely unsafe currently. */ static void * -internal_load_library(const char *libname) +internal_load_library(const char *libname, const char *gucname) { DynamicFileList *file_scanner; PGModuleMagicFunction magic_func; @@ -206,6 +216,7 @@ internal_load_library(const char *libname) if (stat(libname, &stat_buf) == -1) ereport(ERROR, (errcode_for_file_access(), + gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0, errmsg("could not access file \"%s\": %m", libname))); @@ -694,7 +705,7 @@ RestoreLibraryState(char *start_address) { while (*start_address != '\0') { - internal_load_library(start_address); + internal_load_library(start_address, NULL); start_address += strlen(start_address) + 1; } } diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index a604432126c..243b8464fc1 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -1829,7 +1829,7 @@ load_libraries(const char *libraries, const char *gucname, bool restricted) expanded = psprintf("$libdir/plugins/%s", filename); filename = expanded; } - load_file(filename, restricted); + load_file_guc(filename, restricted, gucname); ereport(DEBUG1, (errmsg_internal("loaded library \"%s\"", filename))); if (expanded) diff --git a/src/include/fmgr.h b/src/include/fmgr.h index b120f5e7fef..c4655c8a5f2 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -745,6 +745,7 @@ extern void *load_external_function(const char *filename, const char *funcname, bool signalNotFound, void **filehandle); extern void *lookup_external_function(void *filehandle, const char *funcname); extern void load_file(const char *filename, bool restricted); +extern void load_file_guc(const char *filename, bool restricted, const char *gucname); extern void **find_rendezvous_variable(const char *varName); extern Size EstimateLibraryStateSpace(void); extern void SerializeLibraryState(Size maxsize, char *start_address); -- 2.34.1 [text/x-diff] 0004-show-the-GUC-source-in-errcontext.patch (3.4K, ../../ZKcg1w4FoYq%[email protected]/5-0004-show-the-GUC-source-in-errcontext.patch) download | inline diff: From 0dabe3b9d43091183563ddeb8a47866c427b9c00 Mon Sep 17 00:00:00 2001 From: Justin Pryzby <[email protected]> Date: Wed, 2 Feb 2022 20:54:49 -0600 Subject: [PATCH 4/4] show the GUC source in errcontext //-os-only: freebsd --- src/backend/utils/fmgr/dfmgr.c | 14 +++++++++++--- src/backend/utils/misc/guc_funcs.c | 26 ++++++++++++++++++++++++++ src/include/utils/guc.h | 1 + 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 9674760fc02..80f4b7bed63 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -34,6 +34,7 @@ #include "lib/stringinfo.h" #include "miscadmin.h" #include "storage/shmem.h" +#include "utils/guc_tables.h" #include "utils/hsearch.h" @@ -214,11 +215,18 @@ internal_load_library(const char *libname, const char *gucname) * Check for same files - different paths (ie, symlink or link) */ if (stat(libname, &stat_buf) == -1) + { + char *errstr = strerror(errno); + int linenum; + char *sourcefile = gucname ? GetConfigSourceFile(gucname, &linenum) : NULL; + ereport(ERROR, (errcode_for_file_access(), gucname ? errcontext("while loading shared libraries for setting \"%s\"", gucname) : 0, - errmsg("could not access file \"%s\": %m", - libname))); + sourcefile ? errcontext("from %s:%d", sourcefile, linenum) : 0, + errmsg("could not access file \"%s\": %s", + libname, errstr))); + } for (file_scanner = file_list; file_scanner != NULL && @@ -282,7 +290,7 @@ internal_load_library(const char *libname, const char *gucname) } else { - /* try to close library */ + /* try to close library */ // Not needed due to ERROR ? // dlclose(file_scanner->handle); free(file_scanner); /* complain */ diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c index 0903ba43a96..3d4b0249886 100644 --- a/src/backend/utils/misc/guc_funcs.c +++ b/src/backend/utils/misc/guc_funcs.c @@ -177,6 +177,32 @@ ExtractSetVariableArgs(VariableSetStmt *stmt) } } + +/* + * Get the source file and line associated with the given option, if it was set + * by a file. + * + * If the option doesn't exist, throw an ereport and don't return. + */ +char * +GetConfigSourceFile(const char *name, int *linenum) +{ + struct config_generic *record; + + record = find_option(name, false, false, ERROR); + + /* Should not happen */ + if (record == NULL) + return NULL; + + if (record->source != PGC_S_FILE) + return NULL; + + *linenum = record->sourceline; + return record->sourcefile; +} + + /* * flatten_set_variable_args * Given a parsenode List as emitted by the grammar for SET, diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index bd45d40c106..93f419d0328 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -362,6 +362,7 @@ extern const char *GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged); extern const char *GetConfigOptionResetString(const char *name); extern int GetConfigOptionFlags(const char *name, bool missing_ok); +extern char *GetConfigSourceFile(const char *name, int *linenum); extern void ProcessConfigFile(GucContext context); extern char *convert_GUC_name_for_parameter_acl(const char *name); extern bool check_GUC_name_for_parameter_acl(const char *name); -- 2.34.1 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: warn if GUC set to an invalid shared library 2023-07-06 20:15 Re: warn if GUC set to an invalid shared library Justin Pryzby <[email protected]> @ 2023-12-28 05:26 ` Shubham Khanna <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Shubham Khanna @ 2023-12-28 05:26 UTC (permalink / raw) To: Justin Pryzby <[email protected]>; +Cc: [email protected]; Tom Lane <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; Bharath Rupireddy <[email protected]>; Cary Huang <[email protected]>; Maciek Sakrejda <[email protected]> On Thu, Dec 28, 2023 at 10:54 AM Justin Pryzby <[email protected]> wrote: > > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <[email protected]> wrote: > > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > > It caused no issue when I changed: > > > > > > > > > > > > /* Check that it's acceptable for the indicated parameter */ > > > > > > if (!parse_and_validate_value(record, name, value, > > > > > > - PGC_S_FILE, ERROR, > > > > > > + PGC_S_TEST, ERROR, > > > > > > &newval, &newextra)) > > > > > > > > > > > > I'm not sure where to go from here. > > > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > > sure what a wider change would look like. > > I'm still hoping. > > > > PGC_S_TEST is a better fit, so my question is whether it's really that > > > simple ? > > > > I've added the trivial change as 0001 and re-opened the patch (which ended > > up in January's CF) > > > > If for some reason it's not really as simple as that, then 001 will > > serve as a "straw-man patch" hoping to elicit discussion on that point. > > > From defdb57fe0ec373c1eea8df42f0e1831b3f9c3cc Mon Sep 17 00:00:00 2001 > > From: Justin Pryzby <[email protected]> > > Date: Fri, 22 Jul 2022 15:52:11 -0500 > > Subject: [PATCH v6 1/4] WIP: test GUCs from ALTER SYSTEM as PGC_S_TEST not > > FILE > > > > WIP: ALTER SYSTEM should use PGC_S_TEST rather than PGC_S_FILE > > > > Since the value didn't come from a file. Or maybe we should have > > another PGC_S_ value for this, or a flag for 'is a test'. > > --- > > src/backend/utils/misc/guc.c | 2 +- > > src/include/utils/guc.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > > index 6f21752b844..ae8810591d6 100644 > > --- a/src/backend/utils/misc/guc.c > > +++ b/src/backend/utils/misc/guc.c > > @@ -4435,7 +4435,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) > > > > /* Check that it's acceptable for the indicated parameter */ > > if (!parse_and_validate_value(record, name, value, > > - PGC_S_FILE, ERROR, > > + PGC_S_TEST, ERROR, > > &newval, &newextra)) > > ereport(ERROR, > > (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > > This is rebased over my own patch to enable checks for > REGRESSION_TEST_NAME_RESTRICTIONS. > I was reviewing the Patch and came across a minor issue that the Patch does not apply on the current Head. Please provide the updated version of the patch. Thanks and Regards, Shubham Khanna. ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: warn if GUC set to an invalid shared library 2023-07-06 20:15 Re: warn if GUC set to an invalid shared library Justin Pryzby <[email protected]> @ 2024-01-07 13:27 ` Justin Pryzby <[email protected]> 1 sibling, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2024-01-07 13:27 UTC (permalink / raw) To: [email protected]; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; David G. Johnston <[email protected]>; Bharath Rupireddy <[email protected]>; Cary Huang <[email protected]>; Maciek Sakrejda <[email protected]>; Shubham Khanna <[email protected]> On Fri, Jul 22, 2022 at 03:26:47PM -0400, Tom Lane wrote: > Hmph. I wonder if we shouldn't change that, because it's a lie. > The value isn't actually coming from the config file, at least > not yet. On Thu, Jul 06, 2023 at 03:15:20PM -0500, Justin Pryzby wrote: > On Sat, Oct 29, 2022 at 10:40 AM Justin Pryzby <[email protected]> wrote: > > > > > On Fri, Sep 02, 2022 at 05:24:58PM -0500, Justin Pryzby wrote: > > > > > > It caused no issue when I changed: > > > > > > > > > > > > /* Check that it's acceptable for the indicated parameter */ > > > > > > if (!parse_and_validate_value(record, name, value, > > > > > > - PGC_S_FILE, ERROR, > > > > > > + PGC_S_TEST, ERROR, > > > > > > &newval, &newextra)) > > > > > > > > > > > > I'm not sure where to go from here. > > > > > > > > > > I'm hoping for some guidance ; this simple change may be naive, but I'm not > > > > > sure what a wider change would look like. > > I'm still hoping. @cfbot: rebased ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-01-07 13:27 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-26 20:37 [PATCH v9 1/8] pg_dump: make CLUSTER ON a separate dump object.. Justin Pryzby <[email protected]> 2023-07-06 20:15 Re: warn if GUC set to an invalid shared library Justin Pryzby <[email protected]> 2023-12-28 05:26 ` Re: warn if GUC set to an invalid shared library Shubham Khanna <[email protected]> 2024-01-07 13:27 ` Re: warn if GUC set to an invalid shared library Justin Pryzby <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox