On Wed, Jun 28, 2023 at 4:30 PM Amit Langote wrote:
>
> Hi,
>
> On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote:
> > On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote:
> > > Alvaro Herrera writes:
> > > > On 2023-Feb-20, Amit Langote wrote:
> > > >> One more thing we could try is come up with
Hi,
On Tue, Feb 21, 2023 at 4:12 PM Amit Langote wrote:
> On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote:
> > Alvaro Herrera writes:
> > > On 2023-Feb-20, Amit Langote wrote:
> > >> One more thing we could try is come up with a postgres_fdw test case,
> > >> because it uses the RelOptInfo.user
On Tue, Feb 21, 2023 at 12:40 AM Tom Lane wrote:
> Alvaro Herrera writes:
> > On 2023-Feb-20, Amit Langote wrote:
> >> One more thing we could try is come up with a postgres_fdw test case,
> >> because it uses the RelOptInfo.userid value for remote-costs-based
> >> path size estimation. But addi
Alvaro Herrera writes:
> On 2023-Feb-20, Amit Langote wrote:
>> One more thing we could try is come up with a postgres_fdw test case,
>> because it uses the RelOptInfo.userid value for remote-costs-based
>> path size estimation. But adding a test case to contrib module's
>> suite test a core plan
On 2023-Feb-20, Amit Langote wrote:
> On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera
> wrote:
> > I tried a few things for a new test case, but I was unable to find
> > anything useful. Maybe an intermediate view, I thought; no dice.
> > Maybe one with a security barrier would do? Anyway, for
On Fri, Feb 17, 2023 at 9:02 PM Alvaro Herrera wrote:
> On 2022-Dec-11, Amit Langote wrote:
> > While staring at the build_simple_rel() bit mentioned above, I
> > realized that this code fails to set userid correctly in the
> > inheritance parent rels that are child relations of subquery parent
>
On 2023-Feb-17, Alvaro Herrera wrote:
> I tried a few things for a new test case, but I was unable to find
> anything useful. Maybe an intermediate view, I thought; no dice.
> Maybe one with a security barrier would do? Anyway, for now I just kept
> what you added in v2.
Sorry, I failed to keep
On 2022-Dec-11, Amit Langote wrote:
> While staring at the build_simple_rel() bit mentioned above, I
> realized that this code fails to set userid correctly in the
> inheritance parent rels that are child relations of subquery parent
> relations, such as UNION ALL subqueries. In that case, instea
On Mon, Feb 13, 2023 at 22:31 Tom Lane wrote:
> Amit Langote writes:
> > On Mon, Feb 13, 2023 at 5:07 Justin Pryzby wrote:
> >> That seems to add various elog()s which are hit frequently by sqlsmith:
>
> > Thanks for the report. I’ll take a look once I’m back at a computer in a
> > few days.
>
Amit Langote writes:
> On Mon, Feb 13, 2023 at 5:07 Justin Pryzby wrote:
>> That seems to add various elog()s which are hit frequently by sqlsmith:
> Thanks for the report. I’ll take a look once I’m back at a computer in a
> few days.
Looks like we already have a diagnosis and fix [1]. I'll g
On Mon, Feb 13, 2023 at 5:07 Justin Pryzby wrote:
> On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> > 0002 contains changes that has to do with changing how we access
> > checkAsUser in some foreign table planning/execution code sites.
> > Thought it might be better to describe it
On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 0002 contains changes that has to do with changing how we access
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.
This was committed as 599b33b94:
Stop
On 2023-Jan-19, Amit Langote wrote:
> It seems that, with the test as written, it's not the partitioned
> table referenced in the view's query that becomes a child of the UNION
> ALL parent subquery, but the subquery itself. The bug being fixed in
> 0002 doesn't affect the planning of this query
On Tue, Jan 17, 2023 at 7:33 PM Alvaro Herrera wrote:
> On 2022-Dec-12, Amit Langote wrote:
> > On Sun, Dec 11, 2022 at 6:25 PM Amit Langote
> > wrote:
> > > I've attached 0001 to remove those extraneous code blocks and add a
> > > comment mentioning that userid need not be recomputed.
> > >
> >
On 2022-Dec-12, Amit Langote wrote:
> On Sun, Dec 11, 2022 at 6:25 PM Amit Langote wrote:
> > I've attached 0001 to remove those extraneous code blocks and add a
> > comment mentioning that userid need not be recomputed.
> >
> > While staring at the build_simple_rel() bit mentioned above, I
> > r
Justin Pryzby writes:
> On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote:
>> Alvaro could you comment on this ?
I believe Alvaro's on vacation for a few days more.
regards, tom lane
On Wed, Dec 21, 2022 at 01:44:11PM -0600, Justin Pryzby wrote:
> Alvaro could you comment on this ?
I added here so it's not forgotten.
https://commitfest.postgresql.org/42/4107/
Alvaro could you comment on this ?
On Sun, Dec 11, 2022 at 6:25 PM Amit Langote wrote:
> I've attached 0001 to remove those extraneous code blocks and add a
> comment mentioning that userid need not be recomputed.
>
> While staring at the build_simple_rel() bit mentioned above, I
> realized that this code fails to set userid correc
On Sun, Dec 11, 2022 at 06:25:48PM +0900, Amit Langote wrote:
> On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby wrote:
> > The original code rechecks rte->checkAsUser with the rte of the parent
> > rel. The patch changed to access onerel instead, but that's not updated
> > after looping to find the
Hi,
On Sun, Dec 11, 2022 at 5:17 AM Justin Pryzby wrote:
> On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> > 0002 contains changes that has to do with changing how we access
> > checkAsUser in some foreign table planning/execution code sites.
> > Thought it might be better to desc
On Tue, Nov 29, 2022 at 10:37:56PM +0900, Amit Langote wrote:
> 0002 contains changes that has to do with changing how we access
> checkAsUser in some foreign table planning/execution code sites.
> Thought it might be better to describe it separately too.
This was committed as 599b33b94:
Stop
On 2022-Dec-07, Amit Langote wrote:
> While doing that, I noticed that I had missed updating at least one
> comment which still says that permission checking is done off of the
> range table. Attached patch fixes that.
Pushed, thanks.
--
Álvaro HerreraBreisgau, Deutschland — https://
On Wed, Dec 7, 2022 at 7:43 PM Alvaro Herrera wrote:
> On 2022-Dec-06, Alvaro Herrera wrote:
> > I have pushed this finally.
> >
> > I made two further changes:
>
> Actually, I made one further change that I forgot to mention -- I
> changed the API of CombineRangeTables once again; the committed p
On 2022-Dec-06, Alvaro Herrera wrote:
> I have pushed this finally.
>
> I made two further changes:
Actually, I made one further change that I forgot to mention -- I
changed the API of CombineRangeTables once again; the committed patch
has it this way:
+/*
+ * CombineRangeTables
+ * Add
On Wed, Dec 7, 2022 at 4:01 PM Amit Langote wrote:
> On Wed, Dec 7, 2022 at 12:19 AM Alvaro Herrera
> wrote:
> > I have pushed this finally.
>>
> > I'll mark this commitfest entry as committed soon; please post the other
> > two patches you had in this series in a new thread.
>
> Will do, thanks
On Wed, Dec 7, 2022 at 12:19 AM Alvaro Herrera wrote:
> I have pushed this finally.
Thanks a lot.
> I made two further changes:
>
> 1. there was no reason to rename ExecCheckPerms_hook, since its
>signature was changing anyway. I reverted it to the original name.
Sure, that makes sense.
>
I have pushed this finally.
I made two further changes:
1. there was no reason to rename ExecCheckPerms_hook, since its
signature was changing anyway. I reverted it to the original name.
2. I couldn't find any reason to expose ExecGetRTEPermissionInfo, and
given that it's a one-line funct
Hello,
On 2022-Dec-02, Amit Langote wrote:
> This sounds like a better idea than adding a new AttrMap, so done this
> way in the attached 0001.
Thanks for doing that! I have pushed it, but I renamed
ri_RootToPartitionMap to ri_RootToChildMap and moved it to another spot
in ResultRelInfo, which
Hi Alvaro,
On Wed, Nov 30, 2022 at 5:32 PM Alvaro Herrera wrote:
> > On Tue, Nov 29, 2022 at 6:27 PM Alvaro Herrera
> > wrote:
> > Thanks, I've merged all. I do wonder that it is only in PlannedStmt
> > that the list is called something that is not "rtepermlist", but I'm
> > fine with it if yo
Hello,
This didn't apply, so I rebased it on current master, excluding the one
I already pushed. No further changes.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto,
no me acuerdo."
Hello
On 2022-Nov-29, Amit Langote wrote:
> Thanks for taking a look and all the fixup patches. Was working on
> that test I said we should add and then was spending some time
> cleaning things up and breaking some things out into their patches,
> mainly for the ease of review.
Right, excellent
On Wed, Nov 30, 2022 at 11:56 AM Amit Langote wrote:
> On Wed, Nov 30, 2022 at 3:04 AM Alvaro Herrera
> wrote:
> > On 2022-Nov-29, Amit Langote wrote:
> >
> > > Maybe, we should have the following there so that the PlannedStmt's
> > > contents don't point into the Query?
> > >
> > > newpermi
On Wed, Nov 30, 2022 at 3:04 AM Alvaro Herrera wrote:
> On 2022-Nov-29, Amit Langote wrote:
>
> > Maybe, we should have the following there so that the PlannedStmt's
> > contents don't point into the Query?
> >
> > newperminfo = copyObject(perminfo);
>
> Hmm, I suppose if we want a separate RT
On 2022-Nov-29, Amit Langote wrote:
> Maybe, we should have the following there so that the PlannedStmt's
> contents don't point into the Query?
>
> newperminfo = copyObject(perminfo);
Hmm, I suppose if we want a separate RTEPermissionInfo node, we should
instead do GetRTEPermissionInfo(rte)
On Mon, Nov 21, 2022 at 9:03 PM Amit Langote wrote:
> On Thu, Nov 10, 2022 at 8:58 PM Alvaro Herrera
> wrote:
> > 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'
On Wed, Nov 16, 2022 at 3:25 AM Tom Lane wrote:
> Amit Langote writes:
> > On Mon, Nov 14, 2022 at 11:57 PM Tom Lane wrote:
> >> + * The new member is identified by the zero-based index of the List
> >> + * element it should go into, and the bit number to be set therein.
>
> > The comment sound
Amit Langote writes:
> On Mon, Nov 14, 2022 at 11:57 PM Tom Lane wrote:
>> + * The new member is identified by the zero-based index of the List
>> + * element it should go into, and the bit number to be set therein.
> The comment sounds a bit ambiguous, especially the ", and the bit
> number to
Alvaro Herrera writes:
> On 2022-Nov-14, Tom Lane wrote:
>> For discussion's sake, here's my current version of that 0004 patch,
>> rewritten to use list-of-bitmapset as the data structure.
> I feel that there should be more commentary that explains what a
> multi-bms is. Not sure where, maybe j
On 2022-Nov-14, Tom Lane wrote:
> For discussion's sake, here's my current version of that 0004 patch,
> rewritten to use list-of-bitmapset as the data structure.
I feel that there should be more commentary that explains what a
multi-bms is. Not sure where, maybe just put it near the function th
On Mon, Nov 14, 2022 at 11:57 PM Tom Lane wrote:
> Amit Langote writes:
> > On Sat, Nov 12, 2022 at 1:46 AM Tom Lane wrote:
> >> The main thing I was wondering about in connection with that
> >> was whether to assume that there could be other future applications
> >> of the logic to perform mul
[ I'm intentionally forking this off as a new thread, so as to
not confuse the cfbot about what's the live patchset on the
ExecRTCheckPerms thread. ]
Amit Langote writes:
> On Sat, Nov 12, 2022 at 1:46 AM Tom Lane wrote:
>> The main thing I was wondering about in connection with that
>> was whet
On Sat, Nov 12, 2022 at 1:46 AM Tom Lane wrote:
> Alvaro Herrera writes:
> > On 2022-Oct-06, Amit Langote wrote:
> >> Actually, List of Bitmapsets turned out to be something that doesn't
> >> just-work with our Node infrastructure, which I found out thanks to
> >> -DWRITE_READ_PARSE_PLAN_TREES.
Alvaro Herrera writes:
> On 2022-Oct-06, Amit Langote wrote:
>> Actually, List of Bitmapsets turned out to be something that doesn't
>> just-work with our Node infrastructure, which I found out thanks to
>> -DWRITE_READ_PARSE_PLAN_TREES. So, I had to go ahead and add
>> first-class support for co
On 2022-Oct-06, Amit Langote wrote:
> Actually, List of Bitmapsets turned out to be something that doesn't
> just-work with our Node infrastructure, which I found out thanks to
> -DWRITE_READ_PARSE_PLAN_TREES. So, I had to go ahead and add
> first-class support for copy/equal/write/read support f
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
2022年10月15日(土) 15:01 Amit Langote :
>
> On Fri, Oct 7, 2022 at 4:31 PM Amit Langote wrote:
> > On Fri, Oct 7, 2022 at 3:49 PM Amit Langote wrote:
> > > On Fri, Oct 7, 2022 at 1:25 PM Amit Langote
> > > wrote:
> > > > On Fri, Oct 7, 2022 at 10:04 AM Amit Langote
> > > > wrote:
> > > > > On Thu
On Wed, Oct 12, 2022 at 10:50 PM Peter Eisentraut
wrote:
> On 06.10.22 15:29, Amit Langote wrote:
> > I tried in the attached 0004. ModifyTable gets a new member
> > extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".
> >
> > Actually, List of Bitmapsets turned out to be something that d
On 06.10.22 15:29, Amit Langote wrote:
I tried in the attached 0004. ModifyTable gets a new member
extraUpdatedColsBitmaps, which is List of Bitmapset "nodes".
Actually, List of Bitmapsets turned out to be something that doesn't
just-work with our Node infrastructure, which I found out thanks t
On Tue, Oct 4, 2022 at 12:54 PM Tom Lane wrote:
> Amit Langote writes:
> > On Thu, Jul 28, 2022 at 6:18 AM Tom Lane wrote:
> >> ... One more thing: maybe we should rethink where to put
> >> extraUpdatedCols. Between the facts that it's not used for
> >> actual permissions checks, and that it's
Amit Langote writes:
> On Thu, Jul 28, 2022 at 6:18 AM Tom Lane wrote:
>> ... One more thing: maybe we should rethink where to put
>> extraUpdatedCols. Between the facts that it's not used for
>> actual permissions checks, and that it's calculated by the
>> rewriter not parser, it doesn't seem l
On Thu, Jul 28, 2022 at 6:18 AM Tom Lane wrote:
> ... One more thing: maybe we should rethink where to put
> extraUpdatedCols. Between the facts that it's not used for
> actual permissions checks, and that it's calculated by the
> rewriter not parser, it doesn't seem like it really belongs
> in R
Hi,
On 2022-09-07 18:23:06 +0900, Amit Langote wrote:
> Attached updated patches.
Thanks to Justin's recent patch (89d16b63527) to add
-DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES
-DRAW_EXPRESSION_COVERAGE_TEST
to the FreeBSD ci task we now see the following:
h
... One more thing: maybe we should rethink where to put
extraUpdatedCols. Between the facts that it's not used for
actual permissions checks, and that it's calculated by the
rewriter not parser, it doesn't seem like it really belongs
in RelPermissionInfo. Should we keep it in RangeTblEntry?
Shou
Amit Langote writes:
> [ v16 patches ]
I took a quick look at this ...
I think that the notion behind MergeRelPermissionInfos, ie that
a single RelPermissionInfo can represent *all* the checks for
a given table OID, is fundamentally wrong. For example, when
merging a view into an outer query th
On Wed, 6 Apr 2022 at 02:27, Greg Stark wrote:
>
> This is failing regression tests. I don't understand how this patch
> could be affecting this test though. Perhaps it's a problem with the
> json patches that were committed recently -- but they don't seem to be
> causing other patches to fail.
I
This is failing regression tests. I don't understand how this patch
could be affecting this test though. Perhaps it's a problem with the
json patches that were committed recently -- but they don't seem to be
causing other patches to fail.
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/js
On Wed, 23 Mar 2022 at 20:03, Amit Langote wrote:
>
> On Mon, Mar 14, 2022 at 4:36 PM Amit Langote wrote:
> > Also needed fixes when rebasing.
>
> Needed another rebase.
I had a look at the v10-0001 patch. I agree that it seems to be a good
idea to separate out the required permission checks rat
On Wed, Mar 23, 2022 at 12:03 AM Amit Langote
wrote:
> On Mon, Mar 14, 2022 at 4:36 PM Amit Langote
> 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
On 2022-Mar-23, Amit Langote wrote:
> 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.
Let's no
On Mon, Jan 17, 2022 at 3:51 AM Amit Langote
wrote:
> On Thu, Jan 13, 2022 at 3:39 PM Amit Langote
> wrote:
> > On Thu, Jan 13, 2022 at 12:10 PM Julien Rouhaud
> wrote:
> > > On Mon, Dec 20, 2021 at 04:13:04PM +0900, Amit Langote wrote:
> > > > Patch 0002 needed a rebase, because a conflicting
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/468
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 GetRelPerm
On Wed, Jul 7, 2021 at 1:41 PM David Rowley wrote:
> On Fri, 2 Jul 2021 at 12:41, Amit Langote 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.
On Fri, 2 Jul 2021 at 12:41, Amit Langote 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 Fri, Jul 2, 2021 at 12:45 AM Tom Lane wrote:
> Amit Langote 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
Amit Langote 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
On Thu, 1 Jul 2021 at 02:58, Amit Langote wrote:
>
> On Wed, Jun 30, 2021 at 23:34 David Rowley wrote:
>> + while ((rti = bms_next_member(checkPermRels, rti)) > 0)
>> {
>> - RangeTblEntry *rte = (RangeTblEntry *) lfirst(l);
>> + RangeTblEntry *rte = (RangeTblEntry *) list_nth(rangeTable, rti -
On Wed, Jun 30, 2021 at 23:34 David Rowley wrote:
> On Thu, 1 Jul 2021 at 01:34, Amit Langote 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_n
On Thu, 1 Jul 2021 at 01:34, Amit Langote 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 =
Hi,
Last year in [1], I had briefly mentioned $subject. I'm starting this
thread to propose a small patch to alleviate the inefficiency of that
case.
As also mentioned in [1], when running -Mprepared benchmarks
(plan_cache_mode = force_generic_plan) using partitioned tables,
ExecRTCheckPerms() t
71 matches
Mail list logo
