Feedback-ID: ia2694551:Fastmail
Date: Thu, 10 Nov 2022 12:58:01 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Ian Lawrence Barwick <barwick@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	David Rowley <dgrowleyml@gmail.com>, Greg Stark <stark@mit.edu>,
	Zhihong Yu <zyu@yugabyte.com>, Julien Rouhaud <rjuju123@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: ExecRTCheckPerms() and many prunable partitions
Message-ID: <20221110115801.qnsaci3viwlcpy42@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="oza3eh5eyt5u5bwx"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+HiwqEYCLRZ2Boq_uK0pjLn_9b8XL-LmwKj7HN5kJOivUkYLg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221110115801.qnsaci3viwlcpy42%40alvherre.pgsql>
Precedence: bulk


--oza3eh5eyt5u5bwx
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello

I've been trying to understand what 0001 does.  One thing that strikes
me is that it seems like it'd be easy to have bugs because of modifying
the perminfo list inadequately.  I couldn't find any cross-check that
some perminfo element that we obtain for a rte does actually match the
relation we wanted to check.  Maybe we could add a test in some central
place that perminfo->relid equals rte->relid?

A related point is that concatenating lists doesn't seem to worry about
not processing one element multiple times and ending up with bogus offsets.
(I suppose you still have to let an element be processed multiple times
in case you have nested subqueries?  I wonder how good is the test
coverage for such scenarios.)

Why do callers of add_rte_to_flat_rtable() have to modify the rte's
perminfoindex themselves, instead of having the function do it for them?
That looks strange.  But also it's odd that flatten_unplanned_rtes
concatenates the two lists after all the perminfoindexes have been
modified, rather than doing both things (adding each RTEs perminfo to
the global list and offsetting the index) as we walk the list, in
flatten_rtes_walker.  It looks like these two actions are disconnected
from one another, but unless I misunderstand, in reality the opposite is
true.

I think the API of ConcatRTEPermissionInfoLists is a bit weird.  Why not
have the function return the resulting list instead, just like
list_append?  It is more verbose, but it seems easier to grok.

Two trivial changes attached.  (Maybe 0002 is not correct, if you're
also trying to reference finalrtepermlist; but in that case I think the
original may have been misleading as well.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

--oza3eh5eyt5u5bwx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="0002-Simplify-comment-a-little-bit.patch.txt"

From 47afb0d3e1cf6fdf77b5ace643ece15e9c618006 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 10 Nov 2022 12:34:01 +0100
Subject: [PATCH 2/2] Simplify comment a little bit

---
 src/include/nodes/parsenodes.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 39e9e82bfc..abcf8ee229 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1026,7 +1026,7 @@ typedef struct RangeTblEntry
 	 * avoid getting an additional, lesser lock.
 	 *
 	 * perminfoindex is 1-based index of the RTEPermissionInfo belonging to
-	 * this RTE in the query's list of RTEPermissionInfos; 0 if permissions
+	 * this RTE in the containing query's rtepermlist; 0 if permissions
 	 * need not be checked for the RTE.
 	 */
 	Oid			relid;			/* OID of the relation */
-- 
2.30.2


--oza3eh5eyt5u5bwx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="0001-fix-typo.patch.txt"

From 682313f8cd4402b4049792e001b4c24ff691a8f1 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 10 Nov 2022 12:33:44 +0100
Subject: [PATCH 1/2] fix typo

---
 src/include/nodes/parsenodes.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 2d648a8fdd..39e9e82bfc 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1187,7 +1187,7 @@ typedef struct RangeTblEntry
  * reference is represented by setting the bit for InvalidAttrNumber.
  *
  * updatedCols is also used in some other places, for example, to determine
- * which triggers to fire and in FDWs to know which changed columns the need
+ * which triggers to fire and in FDWs to know which changed columns they need
  * to ship off.
  *
  * Generated columns that are caused to be updated by an update to a base
-- 
2.30.2


--oza3eh5eyt5u5bwx--