Fix for REFRESH MATERIALIZED VIEW ownership error message
Hi,
I Initially pointed out here[1]/messages/by-id/55498B5B-0155-4B0E-9B97-23167F8CB380@excoventures.com </messages/by-id/55498B5B-0155-4B0E-9B97-23167F8CB380@excoventures.com> that running REFRESH MATERIALIZED VIEW as a
non-superuser or table owner yields the following message:
test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blah
The error message should say "...owner of materialized view..."
The attached patch corrects this by setting the "relkind" for the
REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
returns the appropriate error message. The updated patch can be tested as such:
CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view b
I'm happy to generate the backpatches for it but wanted to receive feedback
first.
Thanks,
Jonathan
[1]: /messages/by-id/55498B5B-0155-4B0E-9B97-23167F8CB380@excoventures.com </messages/by-id/55498B5B-0155-4B0E-9B97-23167F8CB380@excoventures.com>
Attachments:
0001-treat-refresh-mat-view-as-mat-view.patchapplication/octet-stream; name=0001-treat-refresh-mat-view-as-mat-view.patch; x-unix-mode=0644Download
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 87f5e95827..a769e38829 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4138,6 +4138,7 @@ RefreshMatViewStmt:
REFRESH MATERIALIZED VIEW opt_concurrently qualified_name opt_with_data
{
RefreshMatViewStmt *n = makeNode(RefreshMatViewStmt);
+ n->relkind = OBJECT_MATVIEW;
n->concurrent = $4;
n->relation = $5;
n->skipData = !($6);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 07ab1a3dde..2998f401bb 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3217,6 +3217,7 @@ typedef struct RefreshMatViewStmt
bool concurrent; /* allow concurrent access? */
bool skipData; /* true for WITH NO DATA */
RangeVar *relation; /* relation to insert into */
+ ObjectType relkind; /* OBJECT_MATVIEW */
} RefreshMatViewStmt;
/* ----------------------
On 2018-Aug-17, Jonathan S. Katz wrote:
Hi,
I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
non-superuser or table owner yields the following message:test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blahThe error message should say "...owner of materialized view..."
The attached patch corrects this by setting the "relkind" for the
REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
returns the appropriate error message. The updated patch can be tested as such:CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view bI'm happy to generate the backpatches for it but wanted to receive feedback
first.
Maybe add your test to some regress/ file?
As it is cosmetic, my inclination would be not to backpatch it.
However, I don't quite see how this patch can possibly fix the problem,
since the new struct member is not used anywhere AFAICT.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Dave Cramer
davec@postgresintl.com
www.postgresintl.com
On Fri, 17 Aug 2018 at 18:30, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2018-Aug-17, Jonathan S. Katz wrote:
Hi,
I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW
as a
non-superuser or table owner yields the following message:
test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blahThe error message should say "...owner of materialized view..."
The attached patch corrects this by setting the "relkind" for the
REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that theaclcheck
returns the appropriate error message. The updated patch can be tested
as such:
CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view bI'm happy to generate the backpatches for it but wanted to receive
feedback
first.
Maybe add your test to some regress/ file?
+1
As it is cosmetic, my inclination would be not to backpatch it.
However, I don't quite see how this patch can possibly fix the problem,
since the new struct member is not used anywhere AFAICT.The only place this is used is in aclcheck_error
case OBJECT_MATVIEW:
msg = gettext_noop("permission denied for materialized view %s");
break;
Dave
On 2018-Aug-17, Dave Cramer wrote:
The only place this is used is in aclcheck_error
case OBJECT_MATVIEW:
msg = gettext_noop("permission denied for materialized view %s");
break;
Yes, but do we pass RefreshMatViewStmt->relkind to that routine? I
don't see that we do. Maybe I misread the code.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, 17 Aug 2018 at 19:35, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2018-Aug-17, Dave Cramer wrote:
The only place this is used is in aclcheck_error
case OBJECT_MATVIEW:
msg = gettext_noop("permission denied for materialized view %s");
break;Yes, but do we pass RefreshMatViewStmt->relkind to that routine? I
don't see that we do. Maybe I misread the code.
Actually the code path that gets executed is:
case OBJECT_MATVIEW:
msg = gettext_noop("must be owner of materialized view %s");
break;
as I have the patch applied and now see:
\c - test
You are now connected to database "test" as user "test".
test=> refresh materialized view b;
ERROR: must be owner of materialized view b
Dave Cramer
On Aug 17, 2018, at 6:30 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Aug-17, Jonathan S. Katz wrote:
Hi,
I Initially pointed out here[1] that running REFRESH MATERIALIZED VIEW as a
non-superuser or table owner yields the following message:test=> REFRESH MATERIALIZED VIEW blah;
ERROR: must be owner of relation blahThe error message should say "...owner of materialized view..."
The attached patch corrects this by setting the "relkind" for the
REFRESH MATERIALIZED VIEW command to be "OBJECT_MATVIEW" so that the aclcheck
returns the appropriate error message. The updated patch can be tested as such:CREATE ROLE bar LOGIN;
CREATE TABLE a (x int);
CREATE MATERIALIZED VIEW b AS SELECT * FROM a;
\c - bar
REFRESH MATERIALIZED VIEW b;
ERROR: must be owner of materialized view bI'm happy to generate the backpatches for it but wanted to receive feedback
first.Maybe add your test to some regress/ file?
Done. Please see attached.
As it is cosmetic, my inclination would be not to backpatch it.
It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that they
must be the owner of the “relational” when in reality it’s the materialized view.
Thanks,
Jonathan
Attachments:
0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU-v2.patchapplication/octet-stream; name=0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU-v2.patch; x-unix-mode=0644Download
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 654ee943be..87d4850c11 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -375,7 +375,7 @@ ProcedureCreate(const char *procedureName,
Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup);
Datum proargnames;
bool isnull;
- const char *prokind_keyword;
+ bool isprocedure;
if (!replace)
ereport(ERROR,
@@ -401,7 +401,8 @@ ProcedureCreate(const char *procedureName,
errdetail("\"%s\" is a window function.", procedureName) :
0)));
- prokind_keyword = (prokind == PROKIND_PROCEDURE ? "PROCEDURE" : "FUNCTION");
+ /* Store if this is a procedure/function to help with errors/hints */
+ isprocedure = (prokind == PROKIND_PROCEDURE);
/*
* Not okay to change the return type of the existing proc, since
@@ -415,12 +416,14 @@ ProcedureCreate(const char *procedureName,
returnsSet != oldproc->proretset)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- prokind == PROKIND_PROCEDURE
- ? errmsg("cannot change whether a procedure has output parameters")
- : errmsg("cannot change return type of existing function"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errmsg("cannot change whether a procedure has output parameters") :
+ errmsg("cannot change return type of existing function"),
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
/*
* If it returns RECORD, check for possible change of record type
@@ -444,9 +447,11 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
errdetail("Row type defined by OUT parameters is different."),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
}
/*
@@ -488,9 +493,11 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change name of input parameter \"%s\"",
old_arg_names[j]),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
}
}
@@ -513,9 +520,11 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot remove parameter defaults from existing function"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
proargdefaults = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
Anum_pg_proc_proargdefaults,
@@ -540,9 +549,11 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change data type of existing parameter default value"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
newlc = lnext(newlc);
}
}
On Saturday, August 18, 2018, Jonathan S. Katz <jkatz@postgresql.org> wrote:
It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that
they
must be the owner of the “relational” when in reality it’s the
materialized view.
Materialized views are a type of relation so it is not wrong, just one of
many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected.
David J.
On Aug 18, 2018, at 5:26 PM, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Saturday, August 18, 2018, Jonathan S. Katz <jkatz@postgresql.org <mailto:jkatz@postgresql.org>> wrote:
It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user that they
must be the owner of the “relational” when in reality it’s the materialized view.Materialized views are a type of relation so it is not wrong, just one of many instances where we generalize to "relation" based in implementation details ins team of being explicit about which type of relation is being affected.
Sure, that’s technically correct, but it’s still confusing to a user,
particularly in this cased since the error comes from running
"REFRESH MATERIALIZED VIEW" which is only applied to materialized views.
For instance, if you try running the command on a table:
CREATE TABLE a (x int);
REFRESH MATERIALIZED VIEW a;
ERROR: "a" is not a materialized view
which is what you would and should expect.
Jonathan
On Sat, 18 Aug 2018 at 17:30, Jonathan S. Katz <jkatz@postgresql.org> wrote:
On Aug 18, 2018, at 5:26 PM, David G. Johnston <david.g.johnston@gmail.com>
wrote:On Saturday, August 18, 2018, Jonathan S. Katz <jkatz@postgresql.org>
wrote:It’s cosmetic, but it’s a cosmetic bug: it incorrectly tells the user
that they
must be the owner of the “relational” when in reality it’s the
materialized view.Materialized views are a type of relation so it is not wrong, just one of
many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected.
So why bother having the error message in the code at all then ? Clearly it
was the intent of the author to use this language, unfortunately there was
no test to prove that it works.
This is a simple fix why push back ? Additionally it clarifies exactly what
the problem is for the user as Jonathan points out.
Dave
Dave Cramer <pg@fastcrypt.com> writes:
This is a simple fix why push back ?
What was being pushed back on, I think, was the claim that this needed to
be back-patched. I'd be inclined not to, since (a) the message is not
wrong, only less specific than it could be, and (b) people tend to get
annoyed by unnecessary behavior changes in released branches.
There might be an argument for putting it into v11, but not further back.
regards, tom lane
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Dave Cramer <pg@fastcrypt.com> writes:
This is a simple fix why push back ?
What was being pushed back on, I think, was the claim that this needed to
be back-patched. I'd be inclined not to, since (a) the message is not
wrong, only less specific than it could be, and (b) people tend to get
annoyed by unnecessary behavior changes in released branches.There might be an argument for putting it into v11, but not further back.
I tend to agree with this, and we don't need to add more work for
translators either, which I'm guessing this would. For v11 and HEAD, as
long as it doesn't make the code ugly, I do think it'd be good to be
more specific.
Thanks!
Stephen
On Aug 18, 2018, at 5:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <pg@fastcrypt.com> writes:
This is a simple fix why push back ?
What was being pushed back on, I think, was the claim that this needed to
be back-patched. I'd be inclined not to, since (a) the message is not
wrong, only less specific than it could be,
I know I’m being pedantic on this one, but “technically” not wrong, it’s still
incomplete to a user. But...
and (b) people tend to get
annoyed by unnecessary behavior changes in released branches.
I will agree with this - thinking about it, people have have coded their apps
to work with the existing message and may have logic around handling it.
I don’t know how likely that is, but I’m willing to err on the side of caution.
There might be an argument for putting it into v11, but not further back.
I’m fine with that.
Thanks,
Jonathan
On Sat, 18 Aug 2018 at 17:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Dave Cramer <pg@fastcrypt.com> writes:
This is a simple fix why push back ?
What was being pushed back on, I think, was the claim that this needed to
be back-patched. I'd be inclined not to, since (a) the message is not
wrong, only less specific than it could be, and (b) people tend to get
annoyed by unnecessary behavior changes in released branches.
I was referring to:
"Materialized views are a type of relation so it is not wrong, just one of
many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected."
As being push back.
I don't have an opinion on back patching this.
Dave Cramer
davec@postgresintl.com
www.postgresintl.com
On Saturday, August 18, 2018, Dave Cramer <pg@fastcrypt.com> wrote:
I was referring to:
"Materialized views are a type of relation so it is not wrong, just one
of many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected."As being push back.
I don't have an opinion on back patching this.
I was arguing against back patching on the basis of defining this as a
bug. It's not wrong nor severe enough to warrant the side effects others
have noted.
David J.
On Sat, Aug 18, 2018 at 03:38:47PM -0700, David G. Johnston wrote:
On Saturday, August 18, 2018, Dave Cramer <pg@fastcrypt.com> wrote:
I was referring to:
"Materialized views are a type of relation so it is not wrong, just one
of many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected."As being push back.
I don't have an opinion on back patching this.
I was arguing against back patching on the basis of defining this as a
bug. It's not wrong nor severe enough to warrant the side effects others
have noted.
I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.
--
Michael
On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Aug 18, 2018 at 03:38:47PM -0700, David G. Johnston wrote:
On Saturday, August 18, 2018, Dave Cramer <pg@fastcrypt.com> wrote:
I was referring to:"Materialized views are a type of relation so it is not wrong, just one
of many instances where we generalize to "relation" based in implementation
details ins team of being explicit about which type of relation is being
affected."As being push back.
I don't have an opinion on back patching this.
I was arguing against back patching on the basis of defining this as a
bug. It's not wrong nor severe enough to warrant the side effects others
have noted.I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.
Oops, too much multitasking. I will attach the correct patch when I get home.
Jonathan
On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote:
On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael@paquier.xyz> wrote:
I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.Oops, too much multitasking. I will attach the correct patch when I get home.
Here is the correct patch, sorry about that. This includes aforementioned
tests.
Jonathan
Attachments:
0001-treat-refresh-mat-view-as-mat-view-v2.patchapplication/octet-stream; name=0001-treat-refresh-mat-view-as-mat-view-v2.patch; x-unix-mode=0644Download
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 87f5e95827..a769e38829 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4138,6 +4138,7 @@ RefreshMatViewStmt:
REFRESH MATERIALIZED VIEW opt_concurrently qualified_name opt_with_data
{
RefreshMatViewStmt *n = makeNode(RefreshMatViewStmt);
+ n->relkind = OBJECT_MATVIEW;
n->concurrent = $4;
n->relation = $5;
n->skipData = !($6);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 07ab1a3dde..2998f401bb 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3217,6 +3217,7 @@ typedef struct RefreshMatViewStmt
bool concurrent; /* allow concurrent access? */
bool skipData; /* true for WITH NO DATA */
RangeVar *relation; /* relation to insert into */
+ ObjectType relkind; /* OBJECT_MATVIEW */
} RefreshMatViewStmt;
/* ----------------------
diff --git a/src/test/regress/expected/matview.out b/src/test/regress/expected/matview.out
index 08cd4bea48..1e4eeb46ee 100644
--- a/src/test/regress/expected/matview.out
+++ b/src/test/regress/expected/matview.out
@@ -589,3 +589,20 @@ SELECT * FROM mvtest2;
ERROR: materialized view "mvtest2" has not been populated
HINT: Use the REFRESH MATERIALIZED VIEW command.
ROLLBACK;
+-- make sure REFRESH MATERIALIZED VIEW errors return that the error occurred on
+-- a "materialized view"
+CREATE ROLE regress_user_mvtest1;
+CREATE ROLE regress_user_mvtest2;
+SET SESSION AUTHORIZATION regress_user_mvtest1;
+CREATE TABLE mvtest_a (x int);
+CREATE MATERIALIZED VIEW mvtest_mv_a AS SELECT * FROM mvtest_a;
+REFRESH MATERIALIZED VIEW mvtest_mv_a;
+SET SESSION AUTHORIZATION regress_user_mvtest2;
+REFRESH MATERIALIZED VIEW mvtest_mv_a;
+ERROR: must be owner of materialized view mvtest_mv_a
+SET SESSION AUTHORIZATION regress_user_mvtest1;
+DROP MATERIALIZED VIEW mvtest_mv_a;
+DROP TABLE mvtest_a;
+RESET SESSION AUTHORIZATION;
+DROP ROLE regress_user_mvtest2;
+DROP ROLE regress_user_mvtest1;
diff --git a/src/test/regress/sql/matview.sql b/src/test/regress/sql/matview.sql
index d96175aa26..349f0f51fd 100644
--- a/src/test/regress/sql/matview.sql
+++ b/src/test/regress/sql/matview.sql
@@ -236,3 +236,20 @@ SELECT mvtest_func();
SELECT * FROM mvtest1;
SELECT * FROM mvtest2;
ROLLBACK;
+
+-- make sure REFRESH MATERIALIZED VIEW errors return that the error occurred on
+-- a "materialized view"
+CREATE ROLE regress_user_mvtest1;
+CREATE ROLE regress_user_mvtest2;
+SET SESSION AUTHORIZATION regress_user_mvtest1;
+CREATE TABLE mvtest_a (x int);
+CREATE MATERIALIZED VIEW mvtest_mv_a AS SELECT * FROM mvtest_a;
+REFRESH MATERIALIZED VIEW mvtest_mv_a;
+SET SESSION AUTHORIZATION regress_user_mvtest2;
+REFRESH MATERIALIZED VIEW mvtest_mv_a;
+SET SESSION AUTHORIZATION regress_user_mvtest1;
+DROP MATERIALIZED VIEW mvtest_mv_a;
+DROP TABLE mvtest_a;
+RESET SESSION AUTHORIZATION;
+DROP ROLE regress_user_mvtest2;
+DROP ROLE regress_user_mvtest1;
On 2018-Aug-18, Jonathan S. Katz wrote:
On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote:
On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael@paquier.xyz> wrote:
I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.Oops, too much multitasking. I will attach the correct patch when I get home.
Here is the correct patch, sorry about that. This includes aforementioned
tests.
I ran the test without the code change, and it passes. I don't think
it's testing what you think it is testing.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Aug 18, 2018, at 11:59 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2018-Aug-18, Jonathan S. Katz wrote:
On Aug 18, 2018, at 8:52 PM, Jonathan S. Katz <jkatz@postgresql.org> wrote:
On Aug 18, 2018, at 8:45 PM, Michael Paquier <michael@paquier.xyz> wrote:
I am not so sure about v11 as it is very close to release, surely we can
do something for HEAD as that's cosmetic. Anyway, if something is
proposed, could a patch be posted? The only patch I am seeing on this
thread refers to improvements for error messages of procedures.Oops, too much multitasking. I will attach the correct patch when I get home.
Here is the correct patch, sorry about that. This includes aforementioned
tests.I ran the test without the code change, and it passes. I don't think
it's testing what you think it is testing.
So I ran the tests against 10.5 unpatched and it failed as expected. I then
ran it against HEAD unpatched and it passed.
Digging into it, it appears the issue was resolved in this commit[1] for 11
and beyond. As the plan is not to backpatch, I’ll withdraw my patch.
Thanks for the help,
Jonathan
https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=8b9e9644dc6a9bd4b7a97950e6212f63880cf18b <https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=8b9e9644dc6a9bd4b7a97950e6212f63880cf18b>