ExecRTCheckPerms() and many prunable partitions

On Thu, 1 Jul 2021 at 01:34, Amit Langote <amitlangote09@gmail.com> wrote:

For now, I have implemented the idea 2 as the attached patch.

I only just had a fleeting glance at the patch. Aren't you
accidentally missing the 0th RTE here?

+ while ((rti = bms_next_member(checkPermRels, rti)) > 0)
  {
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+ RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti - 1);

I'd have expected >= 0 rather than > 0.

David

On Wed, Jun 30, 2021 at 23:34 David Rowley <dgrowleyml@gmail.com> wrote:

On Thu, 1 Jul 2021 at 01:34, Amit Langote <amitlangote09@gmail.com> wrote:

For now, I have implemented the idea 2 as the attached patch.

I only just had a fleeting glance at the patch. Aren't you
accidentally missing the 0th RTE here?

+ while ((rti = bms_next_member(checkPermRels, rti)) > 0)
{
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+ RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti - 1);

I'd have expected >= 0 rather than > 0.

Hmm, a valid RT index cannot be 0, so that seems fine to me. Note that RT
indexes are added as-is to that bitmapset, not after subtracting 1.

--

Amit Langote
EDB: http://www.enterprisedb.com

On Thu, 1 Jul 2021 at 02:58, Amit Langote <amitlangote09@gmail.com> wrote:

On Wed, Jun 30, 2021 at 23:34 David Rowley <dgrowleyml@gmail.com> wrote:

+ while ((rti = bms_next_member(checkPermRels, rti)) > 0)
{
- RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
+ RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti - 1);

I'd have expected >= 0 rather than > 0.

Hmm, a valid RT index cannot be 0, so that seems fine to me. Note that RT indexes are added as-is to that bitmapset, not after subtracting 1.

Oh, you're right. My mistake.

David

Amit Langote <amitlangote09@gmail.com> writes:

The problem is that it loops over the entire range table even though
only one or handful of those entries actually need their permissions
checked. Most entries, especially those of partition child tables
have their requiredPerms set to 0, which David pointed out to me in
[2], so what ExecCheckRTPerms() does in their case is pure overhead.

An idea to fix that is to store the RT indexes of the entries that
have non-0 requiredPerms into a separate list or a bitmapset in
PlannedStmt.

I think perhaps we ought to be more ambitious than that, and consider
separating the list of permissions-to-check from the rtable entirely.
Your patch hardly qualifies as non-invasive, plus it seems to invite
errors of omission, while if we changed the data structure altogether
then the compiler would help find any not-updated code.

But the main reason that this strikes me as possibly a good idea
is that I was just taking another look at the complaint in [1]/messages/by-id/797aff54-b49b-4914-9ff9-aa42564a4d7d@www.fastmail.com,
where I wrote

I think it's impossible to avoid less-than-O(N^2) growth on this sort
of case. For example, the v2 subquery initially has RTEs for v2 itself
plus v1. When we flatten v1 into v2, v2 acquires the RTEs from v1,
namely v1 itself plus foo. Similarly, once vK-1 is pulled up into vK,
there are going to be order-of-K entries in vK's rtable, and that stacking
makes for O(N^2) work overall just in manipulating the rtable.

We can't get rid of these rtable entries altogether, since all of them
represent table privilege checks that the executor will need to do.

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Or maybe not. But I think we should take a hard look at whether
separating these data structures could solve both of these problems
at once.

regards, tom lane

[1]: /messages/by-id/797aff54-b49b-4914-9ff9-aa42564a4d7d@www.fastmail.com

On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Langote <amitlangote09@gmail.com> writes:

The problem is that it loops over the entire range table even though
only one or handful of those entries actually need their permissions
checked. Most entries, especially those of partition child tables
have their requiredPerms set to 0, which David pointed out to me in
[2], so what ExecCheckRTPerms() does in their case is pure overhead.

An idea to fix that is to store the RT indexes of the entries that
have non-0 requiredPerms into a separate list or a bitmapset in
PlannedStmt.

I think perhaps we ought to be more ambitious than that, and consider
separating the list of permissions-to-check from the rtable entirely.
Your patch hardly qualifies as non-invasive, plus it seems to invite
errors of omission, while if we changed the data structure altogether
then the compiler would help find any not-updated code.

But the main reason that this strikes me as possibly a good idea
is that I was just taking another look at the complaint in [1],
where I wrote

I think it's impossible to avoid less-than-O(N^2) growth on this sort
of case. For example, the v2 subquery initially has RTEs for v2 itself
plus v1. When we flatten v1 into v2, v2 acquires the RTEs from v1,
namely v1 itself plus foo. Similarly, once vK-1 is pulled up into vK,
there are going to be order-of-K entries in vK's rtable, and that stacking
makes for O(N^2) work overall just in manipulating the rtable.

We can't get rid of these rtable entries altogether, since all of them
represent table privilege checks that the executor will need to do.

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Or maybe not. But I think we should take a hard look at whether
separating these data structures could solve both of these problems
at once.

Ah, okay. I'll think about decoupling the permission checking stuff
from the range table data structure.

Thanks for the feedback.

I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Fri, 2 Jul 2021 at 12:41, Amit Langote <amitlangote09@gmail.com> wrote:

I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.

I've set it to waiting on author. It was still set to needs review.

If you think you'll not get time to write the patch during this CF,
feel free to bump it out.

David

On Wed, Jul 7, 2021 at 1:41 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, 2 Jul 2021 at 12:41, Amit Langote <amitlangote09@gmail.com> wrote:

I'll mark the CF entry as WoA, unless you'd rather I just mark it RwF.

I've set it to waiting on author. It was still set to needs review.

Sorry it slipped my mind to do that and thanks.

If you think you'll not get time to write the patch during this CF,
feel free to bump it out.

I will try to post an update next week if not later this week,
hopefully with an updated patch.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think perhaps we ought to be more ambitious than that, and consider
separating the list of permissions-to-check from the rtable entirely.
Your patch hardly qualifies as non-invasive, plus it seems to invite
errors of omission, while if we changed the data structure altogether
then the compiler would help find any not-updated code.

But the main reason that this strikes me as possibly a good idea
is that I was just taking another look at the complaint in [1],
where I wrote

I think it's impossible to avoid less-than-O(N^2) growth on this sort
of case. For example, the v2 subquery initially has RTEs for v2 itself
plus v1. When we flatten v1 into v2, v2 acquires the RTEs from v1,
namely v1 itself plus foo. Similarly, once vK-1 is pulled up into vK,
there are going to be order-of-K entries in vK's rtable, and that stacking
makes for O(N^2) work overall just in manipulating the rtable.

We can't get rid of these rtable entries altogether, since all of them
represent table privilege checks that the executor will need to do.

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Or maybe not. But I think we should take a hard look at whether
separating these data structures could solve both of these problems
at once.

Ah, okay. I'll think about decoupling the permission checking stuff
from the range table data structure.

I have finished with the attached. Sorry about the delay.

Think I've managed to get the first part done -- getting the
permission-checking info out of the range table -- but have not
seriously attempted the second -- doing away with the OLD/NEW range
table entries in the view/rule action queries, assuming that is what
you meant in the quoted.

One design point I think might need reconsidering is that the list of
the new RelPermissionInfo nodes that holds the permission-checking
info for relations has to be looked up with a linear search using the
relation OID, whereas it was basically free before if a particular of
code had the RTE handy. Maybe I need to check if the overhead of that
is noticeable in some cases.

As there's not much time left in this CF, I've bumped the entry to the next one.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Think I've managed to get the first part done -- getting the
permission-checking info out of the range table -- but have not
seriously attempted the second -- doing away with the OLD/NEW range
table entries in the view/rule action queries, assuming that is what
you meant in the quoted.

I took a stab at the 2nd part, implemented in the attached 0002.

The patch removes UpdateRangeTableOfViewParse() which would add the
dummy OLD/NEW entries to a view rule's action query's rtable, citing
these reasons:

- * These extra RT entries are not actually used in the query,
- * except for run-time locking and permission checking.

0001 makes them unnecessary for permission checking. Though, a
RELATION-kind RTE still be must be present in the rtable for run-time
locking, so I adjusted ApplyRetrieveRule() as follows:

@@ -1803,16 +1804,26 @@ ApplyRetrieveRule(Query *parsetree,
* original RTE to a subquery RTE.
*/
rte = rt_fetch(rt_index, parsetree->rtable);
+ subquery_rte = rte;

-   rte->rtekind = RTE_SUBQUERY;
-   rte->subquery = rule_action;
-   rte->security_barrier = RelationIsSecurityView(relation);
+   /*
+    * Before modifying, store a copy of itself so as to serve as the entry
+    * to be used by the executor to lock the view relation and for the
+    * planner to be able to record the view relation OID in the PlannedStmt
+    * that it produces for the query.
+    */
+   rte = copyObject(rte);
+   parsetree->rtable = lappend(parsetree->rtable, rte);
+
+   subquery_rte->rtekind = RTE_SUBQUERY;
+   subquery_rte->subquery = rule_action;
+   subquery_rte->security_barrier = RelationIsSecurityView(relation);
    /* Clear fields that should not be set in a subquery RTE */
-   rte->relid = InvalidOid;
-   rte->relkind = 0;
-   rte->rellockmode = 0;
-   rte->tablesample = NULL;
-   rte->inh = false;           /* must not be set for a subquery */
+   subquery_rte->relid = InvalidOid;
+   subquery_rte->relkind = 0;
+   subquery_rte->rellockmode = 0;
+   subquery_rte->tablesample = NULL;
+   subquery_rte->inh = false;          /* must not be set for a subquery */

return parsetree;
}

Outputs for a bunch of regression tests needed to be adjusted to
account for that pg_get_viewdef() no longer qualifies view column
names in the deparsed queries, that is, if they reference only a
single relation. Previously, those dummy OLD/NEW entries tricked
make_ruledef(), get_query_def() et al into setting
deparse_context.varprefix to true. contrib/postgre_fdw test output
likewise needed adjustment due to its deparse code being impacted by
those dummy entries no longer being present, I believe.

I haven't yet checked how this further improves the performance for
the case discussed at [1]/messages/by-id/797aff54-b49b-4914-9ff9-aa42564a4d7d@www.fastmail.com that prompted this.

--
Amit Langote
EDB: http://www.enterprisedb.com

[1]: /messages/by-id/797aff54-b49b-4914-9ff9-aa42564a4d7d@www.fastmail.com

On Fri, Aug 20, 2021 at 10:46 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:

On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Perhaps, if we separated the rtable from the required-permissions data
structure, then we could avoid pulling up otherwise-useless RTEs when
flattening a view (or even better, not make the extra RTEs in the
first place??), and thus possibly avoid that exponential planning-time
growth for nested views.

Think I've managed to get the first part done -- getting the
permission-checking info out of the range table -- but have not
seriously attempted the second -- doing away with the OLD/NEW range
table entries in the view/rule action queries, assuming that is what
you meant in the quoted.

I took a stab at the 2nd part, implemented in the attached 0002.

The patch removes UpdateRangeTableOfViewParse() which would add the
dummy OLD/NEW entries to a view rule's action query's rtable

I haven't yet checked how this further improves the performance for
the case discussed at [1] that prompted this.

[1] /messages/by-id/797aff54-b49b-4914-9ff9-aa42564a4d7d@www.fastmail.com

I checked the time required to do explain select * from v512 (worst
case), using the setup described at the above link and I get the
following numbers:

HEAD: 119.774 ms
0001 : 129.802 ms
0002 : 109.456 ms

So it appears that applying only 0001 makes things a bit worse for
this case. That seems to have to do with the following addition in
pull_up_simple_subquery():

@@ -1131,6 +1131,9 @@ pull_up_simple_subquery(PlannerInfo *root, Node
*jtnode, RangeTblEntry *rte,
*/
parse->rtable = list_concat(parse->rtable, subquery->rtable);

+   parse->relpermlist = MergeRelPermissionInfos(parse->relpermlist,
+                                                subquery->relpermlist);
+

What it does is pull up the RelPermissionInfo nodes in the subquery
being pulled up into the parent query and it's not a simple
list_concat(), because I decided that it's better to de-duplicate the
entries for a given relation OID even across subqueries.

Things get better than HEAD with 0002, because less work needs to be
done in the rewriter when copying the subqueries into the main query,
especially the range table, which only has 1 entry now, not 3 per
view.

Attached updated patches. I wrote a longer commit message for 0002 this time.

--
Amit Langote
EDB: http://www.enterprisedb.com

Got this warning:

/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c: In function 'GetResultRelCheckAsUser':
/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c:1898:7: warning: unused variable 'result' [-Wunused-variable]
Oid result;
^~~~~~

I think the idea that GetRelPermissionInfo always has to scan the
complete list by OID is a nonstarter. Maybe it would be possible to
store the list index of the PermissionInfo element in the RelOptInfo or
the RTE? Maybe use special negative values if unknown (it knows to
search the first time) or known non-existant (probably a coding error
condition, maybe not necessary to have this)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Thanks Alvaro for taking a look at this.

On Tue, Sep 7, 2021 at 4:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Got this warning:

/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c: In function 'GetResultRelCheckAsUser':
/pgsql/source/master/contrib/postgres_fdw/postgres_fdw.c:1898:7: warning: unused variable 'result' [-Wunused-variable]
Oid result;
^~~~~~

Fixed.

I think the idea that GetRelPermissionInfo always has to scan the
complete list by OID is a nonstarter. Maybe it would be possible to
store the list index of the PermissionInfo element in the RelOptInfo or
the RTE? Maybe use special negative values if unknown (it knows to
search the first time) or known non-existant (probably a coding error
condition, maybe not necessary to have this)

I implemented this by adding an Index field in RangeTblEntry, because
GetRelPermissionInfo() is used in all phases of query processing and
only RTEs exist from start to end. I did have to spend some time
getting that approach right (get `make check` to pass!), especially to
ensure that the indexes remain in sync during the merging of
RelPermissionInfo across subqueries. The comments I wrote around
GetRelPermissionInfo(), MergeRelPermissionInfos() functions should
hopefully make things clear. Though, I do have a slightly uneasy
feeling around the fact that RTEs now store information that is
computed using some non-trivial logic, whereas most other fields are
simple catalog state or trivial details extracted from how the query
is spelled out by the user.

I also noticed that setrefs.c: add_rtes_to_flat_rtable() was still
doing things -- adding dead subquery RTEs and any RTEs referenced in
the underlying subquery to flat rtable -- that the new approach of
permissions handling makes unnecessary. I fixed that oversight in the
updated patch. A benefit from that simplification is that there is
now a single loop over rtable in that function rather than two that
were needed before.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Fri, Sep 10, 2021 at 12:22 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Tue, Sep 7, 2021 at 4:35 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I think the idea that GetRelPermissionInfo always has to scan the
complete list by OID is a nonstarter. Maybe it would be possible to
store the list index of the PermissionInfo element in the RelOptInfo or
the RTE? Maybe use special negative values if unknown (it knows to
search the first time) or known non-existant (probably a coding error
condition, maybe not necessary to have this)

I implemented this by adding an Index field in RangeTblEntry, because
GetRelPermissionInfo() is used in all phases of query processing and
only RTEs exist from start to end. I did have to spend some time
getting that approach right (get `make check` to pass!), especially to
ensure that the indexes remain in sync during the merging of
RelPermissionInfo across subqueries. The comments I wrote around
GetRelPermissionInfo(), MergeRelPermissionInfos() functions should
hopefully make things clear. Though, I do have a slightly uneasy
feeling around the fact that RTEs now store information that is
computed using some non-trivial logic, whereas most other fields are
simple catalog state or trivial details extracted from how the query
is spelled out by the user.

I also noticed that setrefs.c: add_rtes_to_flat_rtable() was still
doing things -- adding dead subquery RTEs and any RTEs referenced in
the underlying subquery to flat rtable -- that the new approach of
permissions handling makes unnecessary. I fixed that oversight in the
updated patch. A benefit from that simplification is that there is
now a single loop over rtable in that function rather than two that
were needed before.

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

--
Amit Langote
EDB: http://www.enterprisedb.com

Hi,

On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

The cfbot reports new conflicts since about a week ago with this patch:

Latest failure: https://cirrus-ci.com/task/4686414276198400 and
https://api.cirrus-ci.com/v1/artifact/task/4686414276198400/regress_diffs/src/test/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out /tmp/cirrus-ci-build/src/test/regress/results/xml.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out	2022-01-12 05:24:02.795477001 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/xml.out	2022-01-12 05:28:20.329086031 +0000
@@ -603,12 +603,12 @@
 CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
 SELECT table_name, view_definition FROM information_schema.views
   WHERE table_name LIKE 'xmlview%' ORDER BY 1;
- table_name |                                                  view_definition
-------------+-------------------------------------------------------------------------------------------------------------------
+ table_name |                                              view_definition
+------------+------------------------------------------------------------------------------------------------------------
  xmlview1   |  SELECT xmlcomment('test'::text) AS xmlcomment;
  xmlview2   |  SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
  xmlview3   |  SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
- xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS "xmlelement"+
+ xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement"     +
             |    FROM emp;
  xmlview5   |  SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
  xmlview6   |  SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/compression.out /tmp/cirrus-ci-build/src/test/regress/results/compression.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/compression.out	2022-01-12 05:24:02.739471690 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/compression.out	2022-01-12 05:28:23.537403929 +0000
@@ -187,7 +187,7 @@
 --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
  x      | text |           |          |         | extended |             |              |
 View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
    FROM cmdata1;

 SELECT pg_column_compression(f1) FROM cmdata1;
@@ -274,7 +274,7 @@
 --------+------+-----------+----------+---------+----------+-------------+--------------+-------------
  x      | text |           |          |         | extended | lz4         |              |
 View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
    FROM cmdata1;

Could you send a rebased patch? In the meantime I'll switch the cf entry to
Waiting on Author.

On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

The cfbot reports new conflicts since about a week ago with this patch:

I had noticed that too and was meaning to send a new version. Thanks
for the reminder.

Latest failure: https://cirrus-ci.com/task/4686414276198400 and
https://api.cirrus-ci.com/v1/artifact/task/4686414276198400/regress_diffs/src/test/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/xml.out /tmp/cirrus-ci-build/src/test/regress/results/xml.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/xml.out      2022-01-12 05:24:02.795477001 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/xml.out       2022-01-12 05:28:20.329086031 +0000
@@ -603,12 +603,12 @@
CREATE VIEW xmlview9 AS SELECT xmlserialize(content 'good' as text);
SELECT table_name, view_definition FROM information_schema.views
WHERE table_name LIKE 'xmlview%' ORDER BY 1;
- table_name |                                                  view_definition
-------------+-------------------------------------------------------------------------------------------------------------------
+ table_name |                                              view_definition
+------------+------------------------------------------------------------------------------------------------------------
xmlview1   |  SELECT xmlcomment('test'::text) AS xmlcomment;
xmlview2   |  SELECT XMLCONCAT('hello'::xml, 'you'::xml) AS "xmlconcat";
xmlview3   |  SELECT XMLELEMENT(NAME element, XMLATTRIBUTES(1 AS ":one:", 'deuce' AS two), 'content&') AS "xmlelement";
- xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(emp.name AS name, emp.age AS age, emp.salary AS pay)) AS "xmlelement"+
+ xmlview4   |  SELECT XMLELEMENT(NAME employee, XMLFOREST(name AS name, age AS age, salary AS pay)) AS "xmlelement"     +
|    FROM emp;
xmlview5   |  SELECT XMLPARSE(CONTENT '<abc>x</abc>'::text STRIP WHITESPACE) AS "xmlparse";
xmlview6   |  SELECT XMLPI(NAME foo, 'bar'::text) AS "xmlpi";
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/compression.out /tmp/cirrus-ci-build/src/test/regress/results/compression.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/compression.out      2022-01-12 05:24:02.739471690 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/compression.out       2022-01-12 05:28:23.537403929 +0000
@@ -187,7 +187,7 @@
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
x      | text |           |          |         | extended |             |              |
View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
FROM cmdata1;

SELECT pg_column_compression(f1) FROM cmdata1;
@@ -274,7 +274,7 @@
--------+------+-----------+----------+---------+----------+-------------+--------------+-------------
x      | text |           |          |         | extended | lz4         |              |
View definition:
- SELECT cmdata1.f1 AS x
+ SELECT f1 AS x
FROM cmdata1;

Could you send a rebased patch? In the meantime I'll switch the cf entry to
Waiting on Author.

Turns out I had never compiled this patch set to exercise xml and lz4
tests, whose output files contained view definitions shown using \d
that also needed to be updated in the 0002 patch.

Fixed in the attached updated version.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Thu, Jan 13, 2022 at 3:39 PM Amit Langote <amitlangote09@gmail.com> wrote:

On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <rjuju123@gmail.com> wrote:

On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

The cfbot reports new conflicts since about a week ago with this patch:
Could you send a rebased patch? In the meantime I'll switch the cf entry to
Waiting on Author.

Turns out I had never compiled this patch set to exercise xml and lz4
tests, whose output files contained view definitions shown using \d
that also needed to be updated in the 0002 patch.

Fixed in the attached updated version.

cfbot tells me it found a conflict when applying v7 on the latest
HEAD. Fixed in the attached v8.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Mon, Jan 17, 2022 at 3:51 AM Amit Langote <amitlangote09@gmail.com>
wrote:

On Thu, Jan 13, 2022 at 3:39 PM Amit Langote <amitlangote09@gmail.com>
wrote:

On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud <rjuju123@gmail.com>

wrote:

On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:

Patch 0002 needed a rebase, because a conflicting change to
expected/rules.out has since been committed.

The cfbot reports new conflicts since about a week ago with this patch:
Could you send a rebased patch? In the meantime I'll switch the cf

entry to

Waiting on Author.

Turns out I had never compiled this patch set to exercise xml and lz4
tests, whose output files contained view definitions shown using \d
that also needed to be updated in the 0002 patch.

Fixed in the attached updated version.

cfbot tells me it found a conflict when applying v7 on the latest
HEAD. Fixed in the attached v8.

Hi,

For patch 02, in the description:

present for locking views during execition

Typo: execution.

+    * to be used by the executor to lock the view relation and for the
+    * planner to be able to record the view relation OID in the PlannedStmt
+    * that it produces for the query.

I think the sentence about executor can be placed after the sentence for
the planner.

For patch 01, GetRelPermissionInfo():

+       return perminfo;
+   }
+   else

keyword 'else' is not needed - the else block can be left-indented.

Cheers

On Tue, Jan 18, 2022 at 12:42 AM Zhihong Yu <zyu@yugabyte.com> wrote:

Hi,
For patch 02, in the description:

Thanks for looking.

present for locking views during execition

Typo: execution.

+    * to be used by the executor to lock the view relation and for the
+    * planner to be able to record the view relation OID in the PlannedStmt
+    * that it produces for the query.

I think the sentence about executor can be placed after the sentence for the planner.

Fixed.

For patch 01, GetRelPermissionInfo():

+       return perminfo;
+   }
+   else

keyword 'else' is not needed - the else block can be left-indented.

OK, done.

Also needed fixes when rebasing.

--
Amit Langote
EDB: http://www.enterprisedb.com

On Mon, Mar 14, 2022 at 4:36 PM Amit Langote <amitlangote09@gmail.com> wrote:

Also needed fixes when rebasing.

Needed another rebase.

As the changes being made with the patch are non-trivial and the patch
hasn't been reviewed very significantly since Alvaro's comments back
in Sept 2021 which I've since addressed, I'm thinking of pushing this
one into the version 16 dev cycle.

--
Amit Langote
EDB: http://www.enterprisedb.com