public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. 3+ messages / 3 participants [nested] [flat]
* [PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. @ 2020-03-30 23:59 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2020-03-30 23:59 UTC (permalink / raw) pg_ls_dir_* will now skip (no longer show) symbolic links, same as other non-regular file types, as we advertize we do since 8b6d94cf6. That seems to be the intented behavior, since irregular file types are 1) less portable; and, 2) we don't currently show a file's type except for "bool is_dir". pg_stat_file will now 1) show metadata of links themselves, rather than their target; and, 2) specifically, show links to directories with "is_dir=false"; and, 3) not error on broken symlinks. --- doc/src/sgml/func.sgml | 2 +- src/backend/utils/adt/genfile.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 9b885102da..96b08d0500 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25486,7 +25486,7 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); size, last accessed time stamp, last modified time stamp, last file status change time stamp (Unix platforms only), file creation time stamp (Windows only), and a <type>boolean</type> - indicating if it is a directory (or a symbolic link to a directory). + indicating if it is a directory. Typical usages include: <programlisting> SELECT * FROM pg_stat_file('filename'); diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index ceaa6180da..219ac160f8 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -370,7 +370,7 @@ pg_stat_file(PG_FUNCTION_ARGS) filename = convert_and_check_filename(filename_t); - if (stat(filename, &fst) < 0) + if (lstat(filename, &fst) < 0) { if (missing_ok && errno == ENOENT) PG_RETURN_NULL(); @@ -596,7 +596,7 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, bool missing_ok) /* Get the file info */ snprintf(path, sizeof(path), "%s/%s", dir, de->d_name); - if (stat(path, &attrib) < 0) + if (lstat(path, &attrib) < 0) { /* Ignore concurrently-deleted files, else complain */ if (errno == ENOENT) -- 2.17.0 --2FkSFaIQeDFoAt0B Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v16-0003-Add-tests-on-pg_ls_dir-before-changing-it.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Add more sanity checks around callers of changeDependencyFor() @ 2023-06-28 23:36 Michael Paquier <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Michael Paquier @ 2023-06-28 23:36 UTC (permalink / raw) To: Postgres hackers <[email protected]> Hi all, While working on a different patch, I have noted three code paths that call changeDependencyFor() but don't check that they do not return errors. In all the three cases (support function, extension/schema and object/schema), it seems to me that only one dependency update is expected. I am adding that to the next CF. Thoughts or comments about the attached? -- Michael Attachments: [text/x-diff] 0001-Add-checks-for-values-returned-by-changeDependencyFo.patch (2.6K, ../../ZJzD%[email protected]/2-0001-Add-checks-for-values-returned-by-changeDependencyFo.patch) download | inline diff: From 2b66afedbfd27a80beae3a48e0eb362e21ef4547 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 29 Jun 2023 08:35:23 +0900 Subject: [PATCH] Add checks for values returned by changeDependencyFor() --- src/backend/commands/alter.c | 6 ++++-- src/backend/commands/extension.c | 6 ++++-- src/backend/commands/functioncmds.c | 10 +++++++--- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index e95dc31bde..455d8dd86a 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -848,8 +848,10 @@ AlterObjectNamespace_internal(Relation rel, Oid objid, Oid nspOid) pfree(replaces); /* update dependencies to point to the new schema */ - changeDependencyFor(classId, objid, - NamespaceRelationId, oldNspOid, nspOid); + if (changeDependencyFor(classId, objid, + NamespaceRelationId, oldNspOid, nspOid) != 1) + elog(ERROR, "failed to change schema dependency for object %u", + objid); InvokeObjectPostAlterHook(classId, objid, 0); diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index 0eabe18335..e95642e14f 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -2948,8 +2948,10 @@ AlterExtensionNamespace(const char *extensionName, const char *newschema, Oid *o table_close(extRel, RowExclusiveLock); /* update dependencies to point to the new schema */ - changeDependencyFor(ExtensionRelationId, extensionOid, - NamespaceRelationId, oldNspOid, nspOid); + if (changeDependencyFor(ExtensionRelationId, extensionOid, + NamespaceRelationId, oldNspOid, nspOid) != 1) + elog(ERROR, "failed to change schema dependency for extension %s", + NameStr(extForm->extname)); InvokeObjectPostAlterHook(ExtensionRelationId, extensionOid, 0); diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 49c7864c7c..5ec2c002d2 100644 --- a/src/backend/commands/functioncmds.c +++ b/src/backend/commands/functioncmds.c @@ -1450,9 +1450,13 @@ AlterFunction(ParseState *pstate, AlterFunctionStmt *stmt) /* Add or replace dependency on support function */ if (OidIsValid(procForm->prosupport)) - changeDependencyFor(ProcedureRelationId, funcOid, - ProcedureRelationId, procForm->prosupport, - newsupport); + { + if (changeDependencyFor(ProcedureRelationId, funcOid, + ProcedureRelationId, procForm->prosupport, + newsupport) != 1) + elog(ERROR, "failed to change support dependency for function %s", + get_func_name(funcOid)); + } else { ObjectAddress referenced; -- 2.40.1 [application/pgp-signature] signature.asc (833B, ../../ZJzD%[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Add more sanity checks around callers of changeDependencyFor() @ 2023-06-29 07:06 Heikki Linnakangas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Heikki Linnakangas @ 2023-06-29 07:06 UTC (permalink / raw) To: Michael Paquier <[email protected]>; Postgres hackers <[email protected]> On 29/06/2023 02:36, Michael Paquier wrote: > Hi all, > > While working on a different patch, I have noted three code paths that > call changeDependencyFor() but don't check that they do not return > errors. In all the three cases (support function, extension/schema > and object/schema), it seems to me that only one dependency update is > expected. Makes sense. > /* update dependencies to point to the new schema */ Suggest: "update dependency ..." in singular, as there should be only one. > if (changeDependencyFor(ExtensionRelationId, extensionOid, > NamespaceRelationId, oldNspOid, nspOid) != 1) > elog(ERROR, "failed to change schema dependency for extension %s", > NameStr(extForm->extname)); The error messages like "failed to change schema dependency for extension" don't conform to the usual error message style. "could not change schema dependency for extension" would be more conformant. I see that you copy-pasted that from existing messages, and we have a bunch of other "failed to" messages in the repository too, so I'm OK with leaving it as it is for now. Or maybe change the wording of all the changeDependencyFor() callers now, and consider all the other "failed to" messages separately later. If changeDependencyFor() returns >= 2, the message is a bit misleading. That's what the existing callers did too, so maybe that's fine. I can hit the above error with the attached test case. That seems wrong, although I don't know if it means that the check is wrong or it exposed a long-standing bug. -- Heikki Linnakangas Neon (https://neon.tech) Attachments: [text/x-patch] fail-to-change-schema-dep-test.patch (766B, ../../[email protected]/2-fail-to-change-schema-dep-test.patch) download | inline diff: diff --git a/src/test/modules/test_extensions/sql/test_extensions.sql b/src/test/modules/test_extensions/sql/test_extensions.sql index f4947e7da6..b6b8d9d31f 100644 --- a/src/test/modules/test_extensions/sql/test_extensions.sql +++ b/src/test/modules/test_extensions/sql/test_extensions.sql @@ -210,6 +210,17 @@ ALTER EXTENSION test_ext_cine UPDATE TO '1.1'; \dx+ test_ext_cine + +-- +CREATE SCHEMA test_func_dep1; +CREATE SCHEMA test_func_dep2; +CREATE EXTENSION test_ext_req_schema1 SCHEMA test_func_dep1; +ALTER FUNCTION test_func_dep1.dep_req1() SET SCHEMA test_func_dep2; + +ALTER EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep2; + +DROP EXTENSION test_ext_req_schema1 CASCADE; + -- -- Test @extschema:extname@ syntax and no_relocate option -- ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-06-29 07:06 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-30 23:59 [PATCH v16 02/10] pg_stat_file and pg_ls_dir_* to use lstat().. Justin Pryzby <[email protected]> 2023-06-28 23:36 Add more sanity checks around callers of changeDependencyFor() Michael Paquier <[email protected]> 2023-06-29 07:06 ` Re: Add more sanity checks around callers of changeDependencyFor() Heikki Linnakangas <[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