Обсуждение: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered
Dear PostgreSQL developers,
first, I love the database! Really. For years. Like a marriage. In a good way. Now, on to the bug.
Summary: If a row-level security policy contains a set returning function, pg_dump returns an incorrect serialization of that policy if the return type of the function was altered after the policy was created.
Affected versions: I was able to reproduce the problem on Ubuntu Linux 22.04 with PostgreSQL versions 13.4, 14.4, and 15beta2.
Steps to reproduce the issue: In the attachments, there is a minimal example to reproduce this bug. Please save both the attached files to a folder of your choice, and then run reproduce-bug.sh
If you prefer not to run the bash script, you can run the following code snippet instead.
createdb test
psql test < schema.sql
pg_dump test > dumped-schema.sql
createdb test_restored
psql test_restored < dumped-schema.sql
In schema.sql, we will:
- Create a table (lines 1 to 8)
- Create a function returning a row of this table (lines 10-22)
- Create two policies using this function (lines 27 to 39)
- Remove two columns of the table (lines 42 to 44)
- Dump the schema to dumped-schema.sql (using pg_dump)
Actual and expected output:
In the dumped schema, the policies are serialized as follows:
CREATE POLICY administrate_accounts ON public.users USING ((EXISTS ( SELECT
FROM public.my_account() my_account(id, first_name, last_name, display_name, is_admin)
WHERE my_account.is_admin)));
CREATE POLICY manage_my_account ON public.users USING ((id IN ( SELECT my_account.id
FROM public.my_account() my_account(id, first_name, last_name, display_name, is_admin))));
Instead of this, I would expect a serialization without an aliased FROM clause, because that's how I wrote these policies in the first place.
CREATE POLICY administrate_accounts ON public.users USING ((EXISTS ( SELECT
FROM public.my_account()
WHERE my_account.is_admin)));
CREATE POLICY manage_my_account ON public.users USING ((id IN ( SELECT my_account.id
FROM public.my_account() )));
As you can see from the output, the outputted table alias contains five columns.
my_account(id, first_name, last_name, display_name, is_admin)
This is wrong. When the policy was added, the table actually had five columns. But when the policy was dumped, the table had only three columns left over. Thus, the table alias should look like this:
my_account(id, display_name, is_admin)
In the end, I fail at restoring both the policies. In both cases, the error message is "table "my_account" has 3 columns available but 5 columns specified"
Further platform details:
- Ubuntu 22.04 LTS
- Linux 5.15.0-41-generic x86_64
- Ubuntu GLIBC 2.35-0ubuntu3
- AMD Ryzen 5 5600G with Radeon Graphics
- 32G RAM
Please ask if I can help with further details. If you open a page for this issue, I would be glad to know about its URL.
All the best,
Timo
-- Timo Stolz Geschäftsführer #gerneperdu Nullachtvierzehn UG (haftungsbeschränkt) Konstanzer Straße 15 10707 Berlin Tel +49 (0)30 284243-87 Fax +49 (0)30 284243-88 Web www.nullachtvierzehn.de Mail timo.stolz@nullachtvierzehn.de Sitz: Berlin Amtsgericht: Charlottenburg Handelsregister: HRB 233776 B Geschäftsführung: Timo Stolz Konto bei der GLS Gemeinschaftsbank eG IBAN: DE18 4306 0967 1256 6159 00 BIC: GENODEM1GLS USt-IdNr.: DE346169698
Вложения
Timo Stolz <timo.stolz@nullachtvierzehn.de> writes:
> *Summary: If a row-level security policy contains a set returning
> function, pg_dump returns an incorrect serialization of that policy if
> the return type of the function was altered after the policy was created.*
This has nothing particularly to do with RLS policies; you can
reproduce the same problem with any stored query that selects from a
function-returning-composite, for instance a view.
We even have a regression test case illustrating the current behavior :-(.
I guess nobody thought hard about the implications for dump-and-restore,
but I agree that they're bad.
> Instead of this, I would expect a serialization without an aliased FROM
> clause, because that's how I wrote these policies in the first place.
You might expect that, but you won't get it. As noted in ruleutils.c,
* For a relation RTE, we need only print the alias column names if any
* are different from the underlying "real" names. For a function RTE,
* always emit a complete column alias list; this is to protect against
* possible instability of the default column names (eg, from altering
* parameter names). For tablefunc RTEs, we never print aliases, ...
What we have to do here is to suppress the aliases for any since-dropped
columns, while keeping the live ones. That's slightly finicky, but there
is existing code that can get the job done. ruleutils just wasn't
considering the possibility that function RTEs might have this problem.
The larger issue that this touches on is that we don't prevent you from
dropping the composite type's column even when the query using the
dependent function has hard references to that column (e.g, it's actually
output by the view). Maybe sometime somebody ought to work on tightening
that up. In the meantime though, it's bad for EXPLAIN or pg_dump to fail
altogether on such cases, so I propose the behavior shown in the attached
patch.
(Even if somebody does add the necessary dependencies later, we'd still
need to cope with the situation in released branches, which might already
have broken views to cope with.)
regards, tom lane
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 4e635561aa..8d832efc62 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -3198,6 +3198,9 @@ expandNSItemAttrs(ParseState *pstate, ParseNamespaceItem *nsitem,
*
* "*" is returned if the given attnum is InvalidAttrNumber --- this case
* occurs when a Var represents a whole tuple of a relation.
+ *
+ * It is caller's responsibility to not call this on a dropped attribute.
+ * (You will get some answer for such cases, but it might not be sensible.)
*/
char *
get_rte_attribute_name(RangeTblEntry *rte, AttrNumber attnum)
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 513bf0f026..ac75704363 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -53,6 +53,7 @@
#include "parser/parse_func.h"
#include "parser/parse_node.h"
#include "parser/parse_oper.h"
+#include "parser/parse_relation.h"
#include "parser/parser.h"
#include "parser/parsetree.h"
#include "rewrite/rewriteHandler.h"
@@ -4323,9 +4324,9 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte,
int j;
/*
- * Extract the RTE's "real" column names. This is comparable to
- * get_rte_attribute_name, except that it's important to disregard dropped
- * columns. We put NULL into the array for a dropped column.
+ * Construct an array of the current "real" column names of the RTE.
+ * real_colnames[] will be indexed by physical column number, with NULL
+ * entries for dropped columns.
*/
if (rte->rtekind == RTE_RELATION)
{
@@ -4352,19 +4353,42 @@ set_relation_column_names(deparse_namespace *dpns, RangeTblEntry *rte,
}
else
{
- /* Otherwise use the column names from eref */
+ /* Otherwise get the column names from eref or expandRTE() */
+ List *colnames;
ListCell *lc;
- ncolumns = list_length(rte->eref->colnames);
+ /*
+ * Functions returning composites have the annoying property that some
+ * of the composite type's columns might have been dropped since the
+ * query was parsed. If possible, use expandRTE() to handle that
+ * case, since it has the tedious logic needed to find out about
+ * dropped columns. However, if we're explaining a plan, then we
+ * don't have rte->functions because the planner thinks that won't be
+ * needed later, and that breaks expandRTE(). So in that case we have
+ * to rely on rte->eref, which may lead us to report a dropped
+ * column's old name; that seems close enough for EXPLAIN's purposes.
+ *
+ * For non-RELATION, non-FUNCTION RTEs, we can just look at rte->eref,
+ * which should be sufficiently up-to-date.
+ */
+ if (rte->rtekind == RTE_FUNCTION && rte->functions != NIL)
+ {
+ /* Since we're not creating Vars, rtindex etc. don't matter */
+ expandRTE(rte, 1, 0, -1, true /* include dropped */ ,
+ &colnames, NULL);
+ }
+ else
+ colnames = rte->eref->colnames;
+
+ ncolumns = list_length(colnames);
real_colnames = (char **) palloc(ncolumns * sizeof(char *));
i = 0;
- foreach(lc, rte->eref->colnames)
+ foreach(lc, colnames)
{
/*
- * If the column name shown in eref is an empty string, then it's
- * a column that was dropped at the time of parsing the query, so
- * treat it as dropped.
+ * If the column name we find here is an empty string, then it's a
+ * dropped column, so change to NULL.
*/
char *cname = strVal(lfirst(lc));
@@ -7301,9 +7325,16 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
elog(ERROR, "invalid attnum %d for relation \"%s\"",
attnum, rte->eref->aliasname);
attname = colinfo->colnames[attnum - 1];
- if (attname == NULL) /* dropped column? */
- elog(ERROR, "invalid attnum %d for relation \"%s\"",
- attnum, rte->eref->aliasname);
+
+ /*
+ * If we find a Var referencing a dropped column, it seems better to
+ * print something (anything) than to fail. In general this should
+ * not happen, but there are specific cases involving functions
+ * returning named composite types where we don't sufficiently enforce
+ * that you can't drop a column that's referenced in some view.
+ */
+ if (attname == NULL)
+ attname = "?dropped?column?";
}
else
{
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 9d4f9011a8..6ee49249fc 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -1600,17 +1600,26 @@ select * from tt14v;
begin;
-- this perhaps should be rejected, but it isn't:
alter table tt14t drop column f3;
--- f3 is still in the view ...
+-- column f3 is still in the view, sort of ...
select pg_get_viewdef('tt14v', true);
- pg_get_viewdef
---------------------------------
- SELECT t.f1, +
- t.f3, +
- t.f4 +
- FROM tt14f() t(f1, f3, f4);
+ pg_get_viewdef
+---------------------------------
+ SELECT t.f1, +
+ t."?dropped?column?" AS f3,+
+ t.f4 +
+ FROM tt14f() t(f1, f4);
(1 row)
--- but will fail at execution
+-- ... and you can even EXPLAIN it ...
+explain (verbose, costs off) select * from tt14v;
+ QUERY PLAN
+----------------------------------------
+ Function Scan on testviewschm2.tt14f t
+ Output: t.f1, t.f3, t.f4
+ Function Call: tt14f()
+(3 rows)
+
+-- but it will fail at execution
select f1, f4 from tt14v;
f1 | f4
-----+----
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 50acfe96e6..949b116625 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -580,9 +580,11 @@ begin;
-- this perhaps should be rejected, but it isn't:
alter table tt14t drop column f3;
--- f3 is still in the view ...
+-- column f3 is still in the view, sort of ...
select pg_get_viewdef('tt14v', true);
--- but will fail at execution
+-- ... and you can even EXPLAIN it ...
+explain (verbose, costs off) select * from tt14v;
+-- but it will fail at execution
select f1, f4 from tt14v;
select * from tt14v;
On Wed, 20 Jul 2022 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > This has nothing particularly to do with RLS policies; you can > reproduce the same problem with any stored query that selects from a > function-returning-composite, for instance a view. > Yep, I reached the same conclusion. > What we have to do here is to suppress the aliases for any since-dropped > columns, while keeping the live ones. That's slightly finicky, but there > is existing code that can get the job done. ruleutils just wasn't > considering the possibility that function RTEs might have this problem. > Agreed. I even came up with a similar patch, but your version looks better. > The larger issue that this touches on is that we don't prevent you from > dropping the composite type's column even when the query using the > dependent function has hard references to that column (e.g, it's actually > output by the view). Maybe sometime somebody ought to work on tightening > that up. In the meantime though, it's bad for EXPLAIN or pg_dump to fail > altogether on such cases, so I propose the behavior shown in the attached > patch. > +1. LGTM. Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Wed, 20 Jul 2022 at 21:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The larger issue that this touches on is that we don't prevent you from
>> dropping the composite type's column even when the query using the
>> dependent function has hard references to that column (e.g, it's actually
>> output by the view). Maybe sometime somebody ought to work on tightening
>> that up. In the meantime though, it's bad for EXPLAIN or pg_dump to fail
>> altogether on such cases, so I propose the behavior shown in the attached
>> patch.
> +1. LGTM.
Pushed, thanks for reviewing!
I think I'll go take a look at the missing-dependency aspect now.
I realized from checking the commit log that we've been putting
off doing that since 2014, if not before. Really should fix it.
regards, tom lane
I wrote:
> I think I'll go take a look at the missing-dependency aspect now.
> I realized from checking the commit log that we've been putting
> off doing that since 2014, if not before. Really should fix it.
Here's a proposed patch for that. I wouldn't consider pushing this
into released branches, but maybe it's not too late for v15?
regards, tom lane
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index cf9ceddff1..e119674b1f 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -74,6 +74,7 @@
#include "commands/sequence.h"
#include "commands/trigger.h"
#include "commands/typecmds.h"
+#include "funcapi.h"
#include "nodes/nodeFuncs.h"
#include "parser/parsetree.h"
#include "rewrite/rewriteRemove.h"
@@ -205,6 +206,8 @@ static void deleteOneObject(const ObjectAddress *object,
static void doDeletion(const ObjectAddress *object, int flags);
static bool find_expr_references_walker(Node *node,
find_expr_references_context *context);
+static void process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
+ find_expr_references_context *context);
static void eliminate_duplicate_dependencies(ObjectAddresses *addrs);
static int object_address_comparator(const void *a, const void *b);
static void add_object_address(ObjectClass oclass, Oid objectId, int32 subId,
@@ -1768,6 +1771,12 @@ find_expr_references_walker(Node *node,
add_object_address(OCLASS_CLASS, rte->relid, var->varattno,
context->addrs);
}
+ else if (rte->rtekind == RTE_FUNCTION)
+ {
+ /* Might need to add a dependency on a composite type's column */
+ /* (done out of line, because it's a bit bulky) */
+ process_function_rte_ref(rte, var->varattno, context);
+ }
/*
* Vars referencing other RTE types require no additional work. In
@@ -2342,6 +2351,65 @@ find_expr_references_walker(Node *node,
(void *) context);
}
+/*
+ * find_expr_references_walker subroutine: handle a Var reference
+ * to an RTE_FUNCTION RTE
+ */
+static void
+process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum,
+ find_expr_references_context *context)
+{
+ int atts_done = 0;
+ ListCell *lc;
+
+ /*
+ * Identify which RangeTblFunction produces this attnum, and see if it
+ * returns a composite type. If so, we'd better make a dependency on the
+ * referenced column of the composite type (or actually, of its associated
+ * relation).
+ */
+ foreach(lc, rte->functions)
+ {
+ RangeTblFunction *rtfunc = (RangeTblFunction *) lfirst(lc);
+
+ if (attnum > atts_done &&
+ attnum <= atts_done + rtfunc->funccolcount)
+ {
+ TupleDesc tupdesc;
+
+ tupdesc = get_expr_result_tupdesc(rtfunc->funcexpr, true);
+ if (tupdesc && tupdesc->tdtypeid != RECORDOID)
+ {
+ /*
+ * Named composite type, so individual columns could get
+ * dropped. Make a dependency on this specific column.
+ */
+ Oid reltype = get_typ_typrelid(tupdesc->tdtypeid);
+
+ Assert(attnum - atts_done <= tupdesc->natts);
+ if (OidIsValid(reltype)) /* can this fail? */
+ add_object_address(OCLASS_CLASS, reltype,
+ attnum - atts_done,
+ context->addrs);
+ return;
+ }
+ /* Nothing to do; function's result type is handled elsewhere */
+ return;
+ }
+ atts_done += rtfunc->funccolcount;
+ }
+
+ /* If we get here, must be looking for the ordinality column */
+ if (rte->funcordinality && attnum == atts_done + 1)
+ return;
+
+ /* this probably can't happen ... */
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_COLUMN),
+ errmsg("column %d of relation \"%s\" does not exist",
+ attnum, rte->eref->aliasname)));
+}
+
/*
* Given an array of dependency references, eliminate any duplicates.
*/
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 32fffa472c..d575aa0066 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7330,9 +7330,9 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
/*
* If we find a Var referencing a dropped column, it seems better to
* print something (anything) than to fail. In general this should
- * not happen, but there are specific cases involving functions
- * returning named composite types where we don't sufficiently enforce
- * that you can't drop a column that's referenced in some view.
+ * not happen, but it used to be possible for some cases involving
+ * functions returning named composite types, and perhaps there are
+ * still bugs out there.
*/
if (attname == NULL)
attname = "?dropped?column?";
diff --git a/src/test/regress/expected/create_view.out b/src/test/regress/expected/create_view.out
index 6ee49249fc..a5db1b4b8e 100644
--- a/src/test/regress/expected/create_view.out
+++ b/src/test/regress/expected/create_view.out
@@ -1597,62 +1597,52 @@ select * from tt14v;
foo | baz | 42
(1 row)
-begin;
--- this perhaps should be rejected, but it isn't:
-alter table tt14t drop column f3;
--- column f3 is still in the view, sort of ...
+alter table tt14t drop column f3; -- fail, view has explicit reference to f3
+ERROR: cannot drop column f3 of table tt14t because other objects depend on it
+DETAIL: view tt14v depends on column f3 of table tt14t
+HINT: Use DROP ... CASCADE to drop the dependent objects too.
+drop view tt14v;
+create view tt14v as select t.f1, t.f4 from tt14f() t;
select pg_get_viewdef('tt14v', true);
- pg_get_viewdef
----------------------------------
- SELECT t.f1, +
- t."?dropped?column?" AS f3,+
- t.f4 +
+ pg_get_viewdef
+--------------------------------
+ SELECT t.f1, +
+ t.f4 +
+ FROM tt14f() t(f1, f3, f4);
+(1 row)
+
+select * from tt14v;
+ f1 | f4
+-----+----
+ foo | 42
+(1 row)
+
+alter table tt14t drop column f3; -- ok
+select pg_get_viewdef('tt14v', true);
+ pg_get_viewdef
+----------------------------
+ SELECT t.f1, +
+ t.f4 +
FROM tt14f() t(f1, f4);
(1 row)
--- ... and you can even EXPLAIN it ...
explain (verbose, costs off) select * from tt14v;
QUERY PLAN
----------------------------------------
Function Scan on testviewschm2.tt14f t
- Output: t.f1, t.f3, t.f4
+ Output: t.f1, t.f4
Function Call: tt14f()
(3 rows)
--- but it will fail at execution
-select f1, f4 from tt14v;
+select * from tt14v;
f1 | f4
-----+----
foo | 42
(1 row)
-select * from tt14v;
-ERROR: attribute 3 of type record has been dropped
-rollback;
-begin;
--- this perhaps should be rejected, but it isn't:
-alter table tt14t alter column f4 type integer using f4::integer;
--- f4 is still in the view ...
-select pg_get_viewdef('tt14v', true);
- pg_get_viewdef
---------------------------------
- SELECT t.f1, +
- t.f3, +
- t.f4 +
- FROM tt14f() t(f1, f3, f4);
-(1 row)
-
--- but will fail at execution
-select f1, f3 from tt14v;
- f1 | f3
------+-----
- foo | baz
-(1 row)
-
-select * from tt14v;
-ERROR: attribute 4 of type record has wrong type
-DETAIL: Table has type integer, but query expects text.
-rollback;
+alter table tt14t alter column f4 type integer using f4::integer; -- fail
+ERROR: cannot alter type of a column used by a view or rule
+DETAIL: rule _RETURN on view tt14v depends on column "f4"
-- check display of whole-row variables in some corner cases
create type nestedcomposite as (x int8_tbl);
create view tt15v as select row(i)::nestedcomposite from int8_tbl i;
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 2334a1321e..2721d5bf62 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2247,15 +2247,13 @@ select * from usersview;
id2 | 2 | email2 | 12 | t | 11 | 2
(2 rows)
-begin;
-alter table users drop column moredrop;
-select * from usersview; -- expect clean failure
-ERROR: attribute 5 of type record has been dropped
-rollback;
-alter table users alter column seq type numeric;
-select * from usersview; -- expect clean failure
-ERROR: attribute 2 of type record has wrong type
-DETAIL: Table has type numeric, but query expects integer.
+alter table users drop column moredrop; -- fail
+ERROR: cannot drop column moredrop of table users because other objects depend on it
+DETAIL: view usersview depends on column moredrop of table users
+HINT: Use DROP ... CASCADE to drop the dependent objects too.
+alter table users alter column seq type numeric; -- fail
+ERROR: cannot alter type of a column used by a view or rule
+DETAIL: rule _RETURN on view usersview depends on column "seq"
drop view usersview;
drop function get_first_user();
drop function get_users();
diff --git a/src/test/regress/sql/create_view.sql b/src/test/regress/sql/create_view.sql
index 949b116625..ec4ca72cac 100644
--- a/src/test/regress/sql/create_view.sql
+++ b/src/test/regress/sql/create_view.sql
@@ -575,33 +575,22 @@ create view tt14v as select t.* from tt14f() t;
select pg_get_viewdef('tt14v', true);
select * from tt14v;
-begin;
+alter table tt14t drop column f3; -- fail, view has explicit reference to f3
--- this perhaps should be rejected, but it isn't:
-alter table tt14t drop column f3;
+drop view tt14v;
+
+create view tt14v as select t.f1, t.f4 from tt14f() t;
--- column f3 is still in the view, sort of ...
select pg_get_viewdef('tt14v', true);
--- ... and you can even EXPLAIN it ...
-explain (verbose, costs off) select * from tt14v;
--- but it will fail at execution
-select f1, f4 from tt14v;
select * from tt14v;
-rollback;
-
-begin;
+alter table tt14t drop column f3; -- ok
--- this perhaps should be rejected, but it isn't:
-alter table tt14t alter column f4 type integer using f4::integer;
-
--- f4 is still in the view ...
select pg_get_viewdef('tt14v', true);
--- but will fail at execution
-select f1, f3 from tt14v;
+explain (verbose, costs off) select * from tt14v;
select * from tt14v;
-rollback;
+alter table tt14t alter column f4 type integer using f4::integer; -- fail
-- check display of whole-row variables in some corner cases
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 7e5cde14c4..c834e82c3a 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -682,12 +682,8 @@ SELECT * FROM ROWS FROM(get_users(), generate_series(10,11)) WITH ORDINALITY;
select * from usersview;
alter table users add column junk text;
select * from usersview;
-begin;
-alter table users drop column moredrop;
-select * from usersview; -- expect clean failure
-rollback;
-alter table users alter column seq type numeric;
-select * from usersview; -- expect clean failure
+alter table users drop column moredrop; -- fail
+alter table users alter column seq type numeric; -- fail
drop view usersview;
drop function get_first_user();
On Thu, 21 Jul 2022 at 20:19, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I wrote: > > I think I'll go take a look at the missing-dependency aspect now. > > I realized from checking the commit log that we've been putting > > off doing that since 2014, if not before. Really should fix it. > > Here's a proposed patch for that. I wouldn't consider pushing this > into released branches, but maybe it's not too late for v15? > That looks reasonable to me. It covers all the cases I could think of to try, and I can't see any loopholes. +1 for applying it to v15. Regards, Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Thu, 21 Jul 2022 at 20:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Here's a proposed patch for that. I wouldn't consider pushing this
>> into released branches, but maybe it's not too late for v15?
> That looks reasonable to me. It covers all the cases I could think of
> to try, and I can't see any loopholes. +1 for applying it to v15.
Thanks for checking it!
I had second thoughts about removing the old test cases: that would
leave us with no test coverage for the executor's defenses against
bad plans. I'm not so foolish as to imagine we'll never introduce
another bug that would reach those defenses. So what I did was to
adjust those cases to manually delete the new pg_depend entries,
allowing us to still test what happens without 'em.
Pushed that way.
regards, tom lane