public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v16 08/10] pg_ls_logdir to ignore error if initial/top dir is missing 10+ messages / 6 participants [nested] [flat]
* [PATCH v16 08/10] pg_ls_logdir to ignore error if initial/top dir is missing @ 2020-03-06 23:23 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Justin Pryzby @ 2020-03-06 23:23 UTC (permalink / raw) --- src/backend/utils/adt/genfile.c | 2 +- src/test/regress/input/tablespace.source | 4 ++++ src/test/regress/output/tablespace.source | 7 +++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index fba63568b6..91bf8c69e9 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -647,7 +647,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) Datum pg_ls_logdir(PG_FUNCTION_ARGS) { - return pg_ls_dir_files(fcinfo, Log_directory, LS_DIR_COMMON); + return pg_ls_dir_files(fcinfo, Log_directory, LS_DIR_COMMON | LS_DIR_MISSING_OK); } /* Function to return the list of files in the WAL directory */ diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source index 0b9cfe615e..2a1268e17c 100644 --- a/src/test/regress/input/tablespace.source +++ b/src/test/regress/input/tablespace.source @@ -16,6 +16,10 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@'; -- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir() SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist'; +-- This tests the missing_ok parameter. If that's not functioning, this would ERROR if the logdir doesn't exist yet. +-- The name='' condition is never true, so the function runs to completion but returns zero rows. +SELECT * FROM pg_ls_logdir() WHERE name='Does not exist'; + -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 1c88e914e3..ba9a3fe29a 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -21,6 +21,13 @@ SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspa ------+------+--------------+------- (0 rows) +-- This tests the missing_ok parameter. If that's not functioning, this would ERROR if the logdir doesn't exist yet. +-- The name='' condition is never true, so the function runs to completion but returns zero rows. +SELECT * FROM pg_ls_logdir() WHERE name='Does not exist'; + name | size | modification | isdir +------+------+--------------+------- +(0 rows) + -- try setting and resetting some properties for the new tablespace ALTER TABLESPACE regress_tblspace SET (random_page_cost = 1.0, seq_page_cost = 1.1); ALTER TABLESPACE regress_tblspace SET (some_nonexistent_parameter = true); -- fail -- 2.17.0 --2FkSFaIQeDFoAt0B Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v16-0009-pg_ls_-dir-to-return-all-the-metadata-from-pg_st.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-10 00:37 Jeff Davis <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Jeff Davis @ 2023-06-10 00:37 UTC (permalink / raw) To: Jeff Davis <[email protected]>; [email protected] On Fri, 2023-06-09 at 15:16 -0700, Jeff Davis wrote: > On Fri, 2023-06-09 at 20:54 +0000, Jeff Davis wrote: > > Fix search_path to a safe value during maintenance operations. > > Looks like this is causing pg_amcheck failures in the buildfarm. > Investigating... It looks related to bt_index_check_internal(), which is called by SQL functions bt_index_check() and bt_index_parent_check(). SQL functions can be called in parallel, so it raises the error: ERROR: cannot set parameters during a parallel operation because commit 05e1737351 added the SetConfigOption() line. Normally those functions would not be called in parallel, but debug_parallel_mode makes that happen. Attached a patch to mark those functions as PARALLEL UNSAFE, which fixes the problem. Alternatively, I could just take out that line, as those SQL functions are not controlled by the MAINTAIN privilege. But for consistency I think it's a good idea to leave it in so that index functions are called with the right search path for amcheck. -- Jeff Davis PostgreSQL Contributor Team - AWS Attachments: [text/x-patch] v1-0001-amcheck-mark-bt_index_check-PARALLEL-UNSAFE.patch (2.4K, ../../[email protected]/2-v1-0001-amcheck-mark-bt_index_check-PARALLEL-UNSAFE.patch) download | inline diff: From 0bc11bbc4b06228d33bd9fc6b29dcc9a25d81151 Mon Sep 17 00:00:00 2001 From: Jeff Davis <[email protected]> Date: Fri, 9 Jun 2023 17:24:53 -0700 Subject: [PATCH v1] amcheck: mark bt_index_check() PARALLEL UNSAFE. These functions call bt_index_check_internal(), which needs to set the right search path before executing any index functions. Discussion: https://postgr.es/m/20230609232446.GA123624@nathanxps13 --- contrib/amcheck/Makefile | 4 +++- contrib/amcheck/amcheck--1.3--1.4.sql | 13 +++++++++++++ contrib/amcheck/amcheck.control | 2 +- 3 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 contrib/amcheck/amcheck--1.3--1.4.sql diff --git a/contrib/amcheck/Makefile b/contrib/amcheck/Makefile index b82f221e50..4896c586f8 100644 --- a/contrib/amcheck/Makefile +++ b/contrib/amcheck/Makefile @@ -7,7 +7,9 @@ OBJS = \ verify_nbtree.o EXTENSION = amcheck -DATA = amcheck--1.2--1.3.sql amcheck--1.1--1.2.sql amcheck--1.0--1.1.sql amcheck--1.0.sql +DATA = amcheck--1.3--1.4.sql amcheck--1.2--1.3.sql amcheck--1.1--1.2.sql \ + amcheck--1.0--1.1.sql amcheck--1.0.sql + PGFILEDESC = "amcheck - function for verifying relation integrity" REGRESS = check check_btree check_heap diff --git a/contrib/amcheck/amcheck--1.3--1.4.sql b/contrib/amcheck/amcheck--1.3--1.4.sql new file mode 100644 index 0000000000..409cd55195 --- /dev/null +++ b/contrib/amcheck/amcheck--1.3--1.4.sql @@ -0,0 +1,13 @@ +/* contrib/amcheck/amcheck--1.3--1.4.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "ALTER EXTENSION amcheck UPDATE TO '1.4'" to load this file. \quit + +-- +-- Mark bt_index_check functions as PARALLEL UNSAFE. +-- +ALTER FUNCTION bt_index_check(regclass) PARALLEL UNSAFE; +ALTER FUNCTION bt_index_check(regclass, boolean) PARALLEL UNSAFE; +ALTER FUNCTION bt_index_parent_check(regclass) PARALLEL UNSAFE; +ALTER FUNCTION bt_index_parent_check(regclass, boolean) PARALLEL UNSAFE; +ALTER FUNCTION bt_index_parent_check(regclass, boolean, boolean) PARALLEL UNSAFE; diff --git a/contrib/amcheck/amcheck.control b/contrib/amcheck/amcheck.control index ab50931f75..e67ace01c9 100644 --- a/contrib/amcheck/amcheck.control +++ b/contrib/amcheck/amcheck.control @@ -1,5 +1,5 @@ # amcheck extension comment = 'functions for verifying relation integrity' -default_version = '1.3' +default_version = '1.4' module_pathname = '$libdir/amcheck' relocatable = true -- 2.34.1 ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-10 05:33 Tom Lane <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Tom Lane @ 2023-06-10 05:33 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Jeff Davis <[email protected]>; [email protected] Jeff Davis <[email protected]> writes: > Attached a patch to mark those functions as PARALLEL UNSAFE, which > fixes the problem. > Alternatively, I could just take out that line, as those SQL functions > are not controlled by the MAINTAIN privilege. But for consistency I > think it's a good idea to leave it in so that index functions are > called with the right search path for amcheck. I concur with the upthread objection that it is way too late in the release cycle to be introducing a breaking change like this. I request that you revert it. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-12 17:05 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Noah Misch @ 2023-06-12 17:05 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Jeff Davis <[email protected]>; Jeff Davis <[email protected]>; [email protected] On Sat, Jun 10, 2023 at 01:33:31AM -0400, Tom Lane wrote: > Jeff Davis <[email protected]> writes: > > Attached a patch to mark those functions as PARALLEL UNSAFE, which > > fixes the problem. > > > Alternatively, I could just take out that line, as those SQL functions > > are not controlled by the MAINTAIN privilege. But for consistency I > > think it's a good idea to leave it in so that index functions are > > called with the right search path for amcheck. > > I concur with the upthread objection that it is way too late in > the release cycle to be introducing a breaking change like this. > I request that you revert it. The timing was not great, but this is fixing a purported defect in an older v16 feature. If the MAINTAIN privilege is actually fine, we're all set for v16. If MAINTAIN does have a material problem that $SUBJECT had fixed, we should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem a different way. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-12 17:33 Robert Haas <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Robert Haas @ 2023-06-12 17:33 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Jeff Davis <[email protected]>; Jeff Davis <[email protected]>; [email protected] On Mon, Jun 12, 2023 at 1:05 PM Noah Misch <[email protected]> wrote: > > I concur with the upthread objection that it is way too late in > > the release cycle to be introducing a breaking change like this. > > I request that you revert it. > > The timing was not great, but this is fixing a purported defect in an older > v16 feature. If the MAINTAIN privilege is actually fine, we're all set for > v16. If MAINTAIN does have a material problem that $SUBJECT had fixed, we > should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem a > different way. I wonder why this commit used pg_catalog, pg_temp rather than just the empty string, as AutoVacWorkerMain does. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-13 00:20 Jeff Davis <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Jeff Davis @ 2023-06-13 00:20 UTC (permalink / raw) To: Robert Haas <[email protected]>; Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Jeff Davis <[email protected]>; [email protected] On Mon, 2023-06-12 at 13:33 -0400, Robert Haas wrote: > I wonder why this commit used pg_catalog, pg_temp rather than just > the > empty string, as AutoVacWorkerMain does. I followed the rules here for "Writing SECURITY DEFINER Functions Safely": https://www.postgresql.org/docs/16/sql-createfunction.html which suggests adding pg_temp at the end (otherwise it is searched first by default). It's not exactly like a SECURITY DEFINER function, but running a maintenance command does switch to the table owner, so the risks are similar. I don't see a problem with the pg_temp schema in non-interactive processes, because we trust the non-interactive processes. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-13 00:39 Jeff Davis <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 2 replies; 10+ messages in thread From: Jeff Davis @ 2023-06-13 00:39 UTC (permalink / raw) To: Noah Misch <[email protected]>; Tom Lane <[email protected]>; +Cc: Jeff Davis <[email protected]>; [email protected] On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote: > The timing was not great, but this is fixing a purported defect in an > older > v16 feature. If the MAINTAIN privilege is actually fine, we're all > set for > v16. If MAINTAIN does have a material problem that $SUBJECT had > fixed, we > should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem > a > different way. Someone with the MAINTAIN privilege on a table can use search_path tricks against the table owner, if the code is susceptible, because maintenance code runs with the privileges of the table owner. I was concerned enough to bring it up on the -security list, and then to -hackers followed by a commit (too late). But perhaps that was paranoia: the practical risk is probably quite low, because a user with the MAINTAIN privilege is likely to be highly trusted. I'd like to hear from others on the topic about the relative risks of shipping with/without the search_path changes. I don't think a full revert of the MAINTAIN privilege is the right thing -- the predefined role is very valuable and many other predefined roles are much more dangerous than pg_maintain is. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-13 00:50 David G. Johnston <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: David G. Johnston @ 2023-06-13 00:50 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Noah Misch <[email protected]>; Tom Lane <[email protected]>; Jeff Davis <[email protected]>; [email protected] On Mon, Jun 12, 2023 at 5:40 PM Jeff Davis <[email protected]> wrote: > On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote: > > The timing was not great, but this is fixing a purported defect in an > > older > > v16 feature. If the MAINTAIN privilege is actually fine, we're all > > set for > > v16. If MAINTAIN does have a material problem that $SUBJECT had > > fixed, we > > should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem > > a > > different way. > > Someone with the MAINTAIN privilege on a table can use search_path > tricks against the table owner, if the code is susceptible, because > maintenance code runs with the privileges of the table owner. > > Only change the search_path if someone other than the table owner or superuser is running the command (which should only be possible via the new MAINTAIN privilege)? David J. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-13 01:31 David G. Johnston <[email protected]> parent: David G. Johnston <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: David G. Johnston @ 2023-06-13 01:31 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Noah Misch <[email protected]>; Tom Lane <[email protected]>; Jeff Davis <[email protected]>; [email protected] <[email protected]> On Monday, June 12, 2023, David G. Johnston <[email protected]> wrote: > On Mon, Jun 12, 2023 at 5:40 PM Jeff Davis <[email protected]> wrote: > >> On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote: >> > The timing was not great, but this is fixing a purported defect in an >> > older >> > v16 feature. If the MAINTAIN privilege is actually fine, we're all >> > set for >> > v16. If MAINTAIN does have a material problem that $SUBJECT had >> > fixed, we >> > should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem >> > a >> > different way. >> >> Someone with the MAINTAIN privilege on a table can use search_path >> tricks against the table owner, if the code is susceptible, because >> maintenance code runs with the privileges of the table owner. >> >> > Only change the search_path if someone other than the table owner or > superuser is running the command (which should only be possible via the new > MAINTAIN privilege)? > On a related note, are we OK with someone using this privilege setting their own default_statistics_target? https://www.postgresql.org/docs/current/runtime-config-query.html#GUC-DEFAULT-STATISTICS-TARGET My prior attempt to open up analyze had brought this up as a reason to avoid having someone besides the table owner allowed to analyze the table. David J. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix search_path to a safe value during maintenance operations. @ 2023-06-13 18:29 Noah Misch <[email protected]> parent: Jeff Davis <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Noah Misch @ 2023-06-13 18:29 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Jeff Davis <[email protected]>; [email protected] On Mon, Jun 12, 2023 at 05:39:40PM -0700, Jeff Davis wrote: > On Mon, 2023-06-12 at 13:05 -0400, Noah Misch wrote: > > The timing was not great, but this is fixing a purported defect in an > > older > > v16 feature. If the MAINTAIN privilege is actually fine, we're all > > set for > > v16. If MAINTAIN does have a material problem that $SUBJECT had > > fixed, we > > should either revert MAINTAIN, un-revert $SUBJECT, or fix the problem > > a > > different way. > > Someone with the MAINTAIN privilege on a table can use search_path > tricks against the table owner, if the code is susceptible, because > maintenance code runs with the privileges of the table owner. > > I was concerned enough to bring it up on the -security list, and then > to -hackers followed by a commit (too late). But perhaps that was > paranoia: the practical risk is probably quite low, because a user with > the MAINTAIN privilege is likely to be highly trusted. > > I'd like to hear from others on the topic about the relative risks of > shipping with/without the search_path changes. I find shipping with the search_path change ($SUBJECT) to be lower risk overall, though both are fairly low-risk. Expect no new errors in non-FULL VACUUM, which doesn't run the relevant kinds of code. Tables not ready for the search_path change in ANALYZE already cause errors in Autovacuum ANALYZE and have since 2018-02 (CVE-2018-1058). Hence, $SUBJECT poses less compatibility risk than the CVE-2018-1058 fix. Best argument for shipping without $SUBJECT: we already have REFERENCES and TRIGGER privilege that tend to let the grantee hijack the table owner's account. Adding MAINTAIN to the list, while sad, is defensible. I still prefer to ship with $SUBJECT, not without. ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-06-13 18:29 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-06 23:23 [PATCH v16 08/10] pg_ls_logdir to ignore error if initial/top dir is missing Justin Pryzby <[email protected]> 2023-06-10 00:37 Re: pgsql: Fix search_path to a safe value during maintenance operations. Jeff Davis <[email protected]> 2023-06-10 05:33 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Tom Lane <[email protected]> 2023-06-12 17:05 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Noah Misch <[email protected]> 2023-06-12 17:33 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Robert Haas <[email protected]> 2023-06-13 00:20 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Jeff Davis <[email protected]> 2023-06-13 00:39 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Jeff Davis <[email protected]> 2023-06-13 00:50 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. David G. Johnston <[email protected]> 2023-06-13 01:31 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. David G. Johnston <[email protected]> 2023-06-13 18:29 ` Re: pgsql: Fix search_path to a safe value during maintenance operations. Noah Misch <[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