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
* Missing can't-assign-to-constant checks in plpgsql @ 2022-04-28 21:52 Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Tom Lane @ 2022-04-28 21:52 UTC (permalink / raw) To: [email protected] I happened to notice that there are a couple of places in plpgsql that will let you assign a new value to a variable that's marked CONSTANT: * We don't complain if an output parameter in a CALL statement is constant. * We don't complain if a refcursor variable is constant, even though OPEN may assign a new value to it. The attached quick-hack patch closes both of these oversights. Perhaps the OPEN change is a little too aggressive, since if you give the refcursor variable some non-null initial value, OPEN won't change it; in that usage a CONSTANT marking could be allowed. But I really seriously doubt that anybody out there is marking such variables as constants, so I thought throwing the error at compile time was better than postponing it to runtime so we could handle that. Regardless of which way we handle that point, I'm inclined to change this only in HEAD. Probably people wouldn't thank us for making the back branches more strict. regards, tom lane PS: I didn't do it here, but I'm kind of tempted to pull out all the cursor-related tests in plpgsql.sql and move them to a new test file under src/pl/plpgsql/src/sql/. They look pretty self-contained, and I doubt they're worth keeping in the core tests. Attachments: [text/x-diff] plpgsql-add-missing-checks-for-constant-1.patch (5.3K, ../../[email protected]/2-plpgsql-add-missing-checks-for-constant-1.patch) download | inline diff: diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out index 7b156f3489..1ec6182a8d 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_call.out +++ b/src/pl/plpgsql/src/expected/plpgsql_call.out @@ -122,6 +122,18 @@ CONTEXT: PL/pgSQL function inline_code_block line 6 at CALL DO LANGUAGE plpgsql $$ +DECLARE + x constant int := 3; + y int := 4; +BEGIN + CALL test_proc6(2, x, y); -- error because x is constant +END; +$$; +ERROR: variable "x" is declared CONSTANT +CONTEXT: PL/pgSQL function inline_code_block line 6 at CALL +DO +LANGUAGE plpgsql +$$ DECLARE x int := 3; y int := 4; diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index ceb011810d..d523d13682 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -331,6 +331,7 @@ static void exec_prepare_plan(PLpgSQL_execstate *estate, static void exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr); static void exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan); static void exec_check_rw_parameter(PLpgSQL_expr *expr); +static void exec_check_assignable(PLpgSQL_execstate *estate, int dno); static bool exec_eval_simple_expr(PLpgSQL_execstate *estate, PLpgSQL_expr *expr, Datum *result, @@ -2342,9 +2343,13 @@ make_callstmt_target(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) if (IsA(n, Param)) { Param *param = (Param *) n; + int dno; /* paramid is offset by 1 (see make_datum_param()) */ - row->varnos[nfields++] = param->paramid - 1; + dno = param->paramid - 1; + /* must check assignability now, because grammar can't */ + exec_check_assignable(estate, dno); + row->varnos[nfields++] = dno; } else { @@ -8242,6 +8247,43 @@ exec_check_rw_parameter(PLpgSQL_expr *expr) } } +/* + * exec_check_assignable --- is it OK to assign to the indicated datum? + * + * This should match pl_gram.y's check_assignable(). + */ +static void +exec_check_assignable(PLpgSQL_execstate *estate, int dno) +{ + PLpgSQL_datum *datum; + + Assert(dno >= 0 && dno < estate->ndatums); + datum = estate->datums[dno]; + switch (datum->dtype) + { + case PLPGSQL_DTYPE_VAR: + case PLPGSQL_DTYPE_PROMISE: + case PLPGSQL_DTYPE_REC: + if (((PLpgSQL_variable *) datum)->isconst) + ereport(ERROR, + (errcode(ERRCODE_ERROR_IN_ASSIGNMENT), + errmsg("variable \"%s\" is declared CONSTANT", + ((PLpgSQL_variable *) datum)->refname))); + break; + case PLPGSQL_DTYPE_ROW: + /* always assignable; member vars were checked at compile time */ + break; + case PLPGSQL_DTYPE_RECFIELD: + /* assignable if parent record is */ + exec_check_assignable(estate, + ((PLpgSQL_recfield *) datum)->recparentno); + break; + default: + elog(ERROR, "unrecognized dtype: %d", datum->dtype); + break; + } +} + /* ---------- * exec_set_found Set the global found variable to true/false * ---------- diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y index 11e86c1609..62eb3f24e6 100644 --- a/src/pl/plpgsql/src/pl_gram.y +++ b/src/pl/plpgsql/src/pl_gram.y @@ -2098,6 +2098,12 @@ stmt_open : K_OPEN cursor_variable PLpgSQL_stmt_open *new; int tok; + /* + * cursor_variable must be assignable, since we might + * change its value at runtime + */ + check_assignable((PLpgSQL_datum *) $2, @2); + new = palloc0(sizeof(PLpgSQL_stmt_open)); new->cmd_type = PLPGSQL_STMT_OPEN; new->lineno = plpgsql_location_to_lineno(@1); diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql index 8108d05060..5028398348 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_call.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql @@ -112,6 +112,18 @@ END; $$; +DO +LANGUAGE plpgsql +$$ +DECLARE + x constant int := 3; + y int := 4; +BEGIN + CALL test_proc6(2, x, y); -- error because x is constant +END; +$$; + + DO LANGUAGE plpgsql $$ diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 33eb5e54b9..1af379d1d5 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -2256,6 +2256,18 @@ select refcursor_test2(20000, 20000) as "Should be false", f | t (1 row) +-- should fail to compile +create function return_constant_refcursor() returns refcursor as $$ +declare + rc constant refcursor; +begin + open rc for select a from rc_test; + return rc; +end +$$ language plpgsql; +ERROR: variable "rc" is declared CONSTANT +LINE 5: open rc for select a from rc_test; + ^ -- -- tests for cursors with named parameter arguments -- diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index bffb7b703b..da56450f0f 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -1933,6 +1933,16 @@ $$ language plpgsql; select refcursor_test2(20000, 20000) as "Should be false", refcursor_test2(20, 20) as "Should be true"; +-- should fail to compile +create function return_constant_refcursor() returns refcursor as $$ +declare + rc constant refcursor; +begin + open rc for select a from rc_test; + return rc; +end +$$ language plpgsql; + -- -- tests for cursors with named parameter arguments -- ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Missing can't-assign-to-constant checks in plpgsql @ 2022-04-28 22:11 Pavel Stehule <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Pavel Stehule @ 2022-04-28 22:11 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> čt 28. 4. 2022 v 23:52 odesílatel Tom Lane <[email protected]> napsal: > I happened to notice that there are a couple of places in plpgsql > that will let you assign a new value to a variable that's marked > CONSTANT: > > * We don't complain if an output parameter in a CALL statement > is constant. > > * We don't complain if a refcursor variable is constant, even > though OPEN may assign a new value to it. > > The attached quick-hack patch closes both of these oversights. > > Perhaps the OPEN change is a little too aggressive, since if > you give the refcursor variable some non-null initial value, > OPEN won't change it; in that usage a CONSTANT marking could > be allowed. But I really seriously doubt that anybody out > there is marking such variables as constants, so I thought > throwing the error at compile time was better than postponing > it to runtime so we could handle that. > > Regardless of which way we handle that point, I'm inclined to > change this only in HEAD. Probably people wouldn't thank us > for making the back branches more strict. > +1 I can implement these checks in plpgsql_check. So possible issues can be detected and fixed on older versions by using plpgsql_check. Regards Pavel > regards, tom lane > > PS: I didn't do it here, but I'm kind of tempted to pull out > all the cursor-related tests in plpgsql.sql and move them to > a new test file under src/pl/plpgsql/src/sql/. They look > pretty self-contained, and I doubt they're worth keeping in > the core tests. > > ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2022-04-28 22:11 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]> 2022-04-28 21:52 Missing can't-assign-to-constant checks in plpgsql Tom Lane <[email protected]> 2022-04-28 22:11 ` Re: Missing can't-assign-to-constant checks in plpgsql Pavel Stehule <[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