Horton

Thread

Commits

  1. Remove useless lookup of root partitioned rel in ExecInitModifyTable().

  2. Don't scan partitioned tables.

  1. Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2017-06-21T07:59:43Z

    Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to 
ExecInitModifyTable:

+   /* The root table RT index is at the head of the partitioned_rels 
list */
+   if (node->partitioned_rels)
+   {
+       Index   root_rti;
+       Oid     root_oid;
+
+       root_rti = linitial_int(node->partitioned_rels);
+       root_oid = getrelid(root_rti, estate->es_range_table);
+       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
+   }

but I noticed that that function doesn't use the relation descriptor at 
all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a 
partitioned table, the relation is opened in that case, but the relation 
descriptor isn't referenced at all in initializing WITH CHECK OPTION 
constraints and/or RETURNING projections.  (The mtstate->resultRelinfo 
array is referenced in those initialization, instead.)  So, I'd like to 
propose to remove this from that function.  Attached is a patch for that.

Best regards,
Etsuro Fujita

  2. Re: Useless code in ExecInitModifyTable

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-21T08:57:55Z

    Fujita-san,

On 2017/06/21 16:59, Etsuro Fujita wrote:
> Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
> ExecInitModifyTable:
> 
> +   /* The root table RT index is at the head of the partitioned_rels list */
> +   if (node->partitioned_rels)
> +   {
> +       Index   root_rti;
> +       Oid     root_oid;
> +
> +       root_rti = linitial_int(node->partitioned_rels);
> +       root_oid = getrelid(root_rti, estate->es_range_table);
> +       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
> +   }
> 
> but I noticed that that function doesn't use the relation descriptor at
> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
> partitioned table, the relation is opened in that case, but the relation
> descriptor isn't referenced at all in initializing WITH CHECK OPTION
> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
> array is referenced in those initialization, instead.)  So, I'd like to
> propose to remove this from that function.  Attached is a patch for that.

Thanks for cleaning that up.  I cannot see any problem in applying the patch.

Regards,
Amit

  3. Re: Useless code in ExecInitModifyTable

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-21T14:33:04Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2017/06/21 16:59, Etsuro Fujita wrote:
>> but I noticed that that function doesn't use the relation descriptor at
>> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
>> partitioned table, the relation is opened in that case, but the relation
>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
>> array is referenced in those initialization, instead.)  So, I'd like to
>> propose to remove this from that function.  Attached is a patch for that.

> Thanks for cleaning that up.  I cannot see any problem in applying the patch.

Actually, isn't ModifyTable.partitioned_rels *always* NIL?  I can't
see anyplace in the planner that sets it differently.  I think we should
flush the field altogether.  Anybody who doesn't want that should at least
provide the missing comment for create_modifytable_path's partitioned_rels
argument (and yes, the fact that that is missing isn't making me any
more favorably disposed...)

			regards, tom lane

  4. Re: Useless code in ExecInitModifyTable

    Robert Haas <robertmhaas@gmail.com> — 2017-06-21T14:52:32Z

    On Wed, Jun 21, 2017 at 10:33 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> On 2017/06/21 16:59, Etsuro Fujita wrote:
>>> but I noticed that that function doesn't use the relation descriptor at
>>> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
>>> partitioned table, the relation is opened in that case, but the relation
>>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>>> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
>>> array is referenced in those initialization, instead.)  So, I'd like to
>>> propose to remove this from that function.  Attached is a patch for that.
>
>> Thanks for cleaning that up.  I cannot see any problem in applying the patch.
>
> Actually, isn't ModifyTable.partitioned_rels *always* NIL?  I can't
> see anyplace in the planner that sets it differently.  I think we should
> flush the field altogether.  Anybody who doesn't want that should at least
> provide the missing comment for create_modifytable_path's partitioned_rels
> argument (and yes, the fact that that is missing isn't making me any
> more favorably disposed...)

It appears to me that create_modifytable_path() has a partitioned_rels
argument and it looks like inheritance_planner not only passes it but
guarantees that it's non-empty when the target is a partitioned table.
You're right that there is a comment missing from the function header,
but it's not too hard to figure out what it should be.  Just consult
the definition of ModifyTable itself:

        /* RT indexes of non-leaf tables in a partition tree */
        List       *partitioned_rels;

The point here is that if we have a partitioned table with a bunch of
descendent tables, the non-leaf partitions are never actually scanned;
there's no file on disk to scan.  Despite the lack of a scan, we still
need to arrange for those relations to be locked.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: Useless code in ExecInitModifyTable

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-21T15:33:11Z

    Robert Haas <robertmhaas@gmail.com> writes:
> It appears to me that create_modifytable_path() has a partitioned_rels
> argument and it looks like inheritance_planner not only passes it but
> guarantees that it's non-empty when the target is a partitioned table.

Oh, somehow I missed the call in inheritance_planner.  Right.

			regards, tom lane

  6. Re: Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2017-06-22T02:25:43Z

    On 2017/06/21 23:52, Robert Haas wrote:

> You're right that there is a comment missing from the function header,
> but it's not too hard to figure out what it should be.  Just consult
> the definition of ModifyTable itself:
> 
>          /* RT indexes of non-leaf tables in a partition tree */
>          List       *partitioned_rels;

I agree on that point, but I think it's better to add the missing 
comment for the create_modifytable_path argument, as proposed in [1].

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/e87c4a6d-23d7-5e7c-e8db-44ed418eb5d1%40lab.ntt.co.jp

  7. Re: Useless code in ExecInitModifyTable

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-06-22T02:53:25Z

    On 2017/06/22 11:25, Etsuro Fujita wrote:
> On 2017/06/21 23:52, Robert Haas wrote:
> 
>> You're right that there is a comment missing from the function header,
>> but it's not too hard to figure out what it should be.  Just consult
>> the definition of ModifyTable itself:
>>
>>          /* RT indexes of non-leaf tables in a partition tree */
>>          List       *partitioned_rels;
> 
> I agree on that point, but I think it's better to add the missing comment
> for the create_modifytable_path argument, as proposed in [1].

Thanks for sharing that link.  I was about to send a patch to add the
comment, but seems like you beat me to it.

Thanks,
Amit

  8. Re: Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2017-09-04T10:05:38Z

    On 2017/06/21 17:57, Amit Langote wrote:
> On 2017/06/21 16:59, Etsuro Fujita wrote:
>> Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
>> ExecInitModifyTable:
>>
>> +   /* The root table RT index is at the head of the partitioned_rels list */
>> +   if (node->partitioned_rels)
>> +   {
>> +       Index   root_rti;
>> +       Oid     root_oid;
>> +
>> +       root_rti = linitial_int(node->partitioned_rels);
>> +       root_oid = getrelid(root_rti, estate->es_range_table);
>> +       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
>> +   }
>>
>> but I noticed that that function doesn't use the relation descriptor at
>> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
>> partitioned table, the relation is opened in that case, but the relation
>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
>> array is referenced in those initialization, instead.)  So, I'd like to
>> propose to remove this from that function.  Attached is a patch for that.
> 
> Thanks for cleaning that up.  I cannot see any problem in applying the patch.

I noticed that the patch needs to be rebased.  Updated patch attached.

Thanks for the review!

Best regards,
Etsuro Fujita

  9. Re: Useless code in ExecInitModifyTable

    Ryan Murphy <ryanfmurphy@gmail.com> — 2017-09-05T03:41:50Z

    Hello!

I downloaded the latest patch "clean-nodeModifyTable-v2.patch"
and tried applying it to HEAD using "git apply <patch-name>".

The only response from git was "fatal: unrecognized input".
I tried this with git 2.5.4 stable that comes native on my mac, and git 2.12 built from source.
In both cases I got the same error.

I know you recently rebased this patch already, but could you please double-check it?
Of course it's possible I'm doing something wrong.

Thanks,
Ryan

The new status of this patch is: Waiting on Author

  10. Re: Useless code in ExecInitModifyTable

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-09-05T03:56:14Z

    Ryan Murphy <ryanfmurphy@gmail.com> writes:
> I downloaded the latest patch "clean-nodeModifyTable-v2.patch"
> and tried applying it to HEAD using "git apply <patch-name>".

> The only response from git was "fatal: unrecognized input".

FWIW, I find that good ol' patch is much more reliable about applying
patches that didn't come from git itself.  In this case

	patch -p1 <~../path/to/clean-nodeModifyTable-v2.patch

seems to work without complaint.

			regards, tom lane

  11. Re: Useless code in ExecInitModifyTable

    Ryan Murphy <ryanfmurphy@gmail.com> — 2017-09-05T03:59:23Z

    > FWIW, I find that good ol' patch is much more reliable about applying
> patches that didn't come from git itself.  In this case
>
>
Thanks, I knew I was missing something!
Ryan

  12. Re: [HACKERS] Useless code in ExecInitModifyTable

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-28T02:18:48Z

    On Tue, Sep 5, 2017 at 12:41 PM, Ryan Murphy <ryanfmurphy@gmail.com> wrote:
> The new status of this patch is: Waiting on Author

This status is misleading, so I switched it back to "needs review"
(please be careful about that!). I can still apply the patch
correctly. Sorry I am not taking the time to have a meaningful opinion
about this patch. The patch passes all regression tests. As I am on
this entry, I am bumping the patch to next CF..
-- 
Michael

  13. Re: [HACKERS] Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2017-11-28T05:03:37Z

    (2017/11/28 11:18), Michael Paquier wrote:
> On Tue, Sep 5, 2017 at 12:41 PM, Ryan Murphy<ryanfmurphy@gmail.com>  wrote:
>> The new status of this patch is: Waiting on Author
>
> This status is misleading, so I switched it back to "needs review"
> (please be careful about that!).

I think I forgot to change that status.  Sorry about that.

> I can still apply the patch
> correctly. Sorry I am not taking the time to have a meaningful opinion
> about this patch. The patch passes all regression tests. As I am on
> this entry, I am bumping the patch to next CF..

Ok, thanks!

Best regards,
Etsuro Fujita

  14. Re: [HACKERS] Useless code in ExecInitModifyTable

    Stephen Frost <sfrost@snowman.net> — 2018-01-15T02:28:18Z

    Fujita-san, Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/06/21 16:59, Etsuro Fujita wrote:
> > Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
> > ExecInitModifyTable:
> > 
> > +   /* The root table RT index is at the head of the partitioned_rels list */
> > +   if (node->partitioned_rels)
> > +   {
> > +       Index   root_rti;
> > +       Oid     root_oid;
> > +
> > +       root_rti = linitial_int(node->partitioned_rels);
> > +       root_oid = getrelid(root_rti, estate->es_range_table);
> > +       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
> > +   }
> > 
> > but I noticed that that function doesn't use the relation descriptor at
> > all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
> > partitioned table, the relation is opened in that case, but the relation
> > descriptor isn't referenced at all in initializing WITH CHECK OPTION
> > constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
> > array is referenced in those initialization, instead.)  So, I'd like to
> > propose to remove this from that function.  Attached is a patch for that.
> 
> Thanks for cleaning that up.  I cannot see any problem in applying the patch.

Seems like this has gotten a review (and quite a bit of down-stream
discussion that seemed pretty positive), and the latest patch still
applies cleanly and passes the regression tests- is there some reason
it's still marked as Needs Review?  Seems like it should really be in
Ready for Committer state.

Amit, if you agree, would you mind going ahead and changing it?

Thanks!

Stephen

  15. Re: [HACKERS] Useless code in ExecInitModifyTable

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-15T02:35:26Z

    On 2018/01/15 11:28, Stephen Frost wrote:
> Fujita-san, Amit,
> 
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> On 2017/06/21 16:59, Etsuro Fujita wrote:
>>> Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
>>> ExecInitModifyTable:
>>>
>>> +   /* The root table RT index is at the head of the partitioned_rels list */
>>> +   if (node->partitioned_rels)
>>> +   {
>>> +       Index   root_rti;
>>> +       Oid     root_oid;
>>> +
>>> +       root_rti = linitial_int(node->partitioned_rels);
>>> +       root_oid = getrelid(root_rti, estate->es_range_table);
>>> +       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
>>> +   }
>>>
>>> but I noticed that that function doesn't use the relation descriptor at
>>> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
>>> partitioned table, the relation is opened in that case, but the relation
>>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>>> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
>>> array is referenced in those initialization, instead.)  So, I'd like to
>>> propose to remove this from that function.  Attached is a patch for that.
>>
>> Thanks for cleaning that up.  I cannot see any problem in applying the patch.
> 
> Seems like this has gotten a review (and quite a bit of down-stream
> discussion that seemed pretty positive), and the latest patch still
> applies cleanly and passes the regression tests- is there some reason
> it's still marked as Needs Review?  Seems like it should really be in
> Ready for Committer state.

+1.

> Amit, if you agree, would you mind going ahead and changing it?

Sure, done.

Thanks,
Amit

  16. Re: [HACKERS] Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-15T08:10:09Z

    (2018/01/15 11:35), Amit Langote wrote:
> On 2018/01/15 11:28, Stephen Frost wrote:
>> Seems like this has gotten a review (and quite a bit of down-stream
>> discussion that seemed pretty positive), and the latest patch still
>> applies cleanly and passes the regression tests- is there some reason
>> it's still marked as Needs Review?  Seems like it should really be in
>> Ready for Committer state.

Thanks for taking care of this, Stephen!

> +1.
>
>> Amit, if you agree, would you mind going ahead and changing it?
>
> Sure, done.

Thanks for reviewing, Amit!

Best regards,
Etsuro Fujita

  17. Re: [HACKERS] Useless code in ExecInitModifyTable

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-01-17T19:46:00Z

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> (2018/01/15 11:35), Amit Langote wrote:
>> On 2018/01/15 11:28, Stephen Frost wrote:
>>> Seems like this has gotten a review (and quite a bit of down-stream
>>> discussion that seemed pretty positive), and the latest patch still
>>> applies cleanly and passes the regression tests- is there some reason
>>> it's still marked as Needs Review?  Seems like it should really be in
>>> Ready for Committer state.
>>> Amit, if you agree, would you mind going ahead and changing it?

>> Sure, done.

> Thanks for reviewing, Amit!

Pushed.  I think the long delay on this is really my fault for having
raised an incorrect objection initially --- apologies for that.

			regards, tom lane

  18. Re: [HACKERS] Useless code in ExecInitModifyTable

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-18T07:18:31Z

    (2018/01/18 4:46), Tom Lane wrote:
> Pushed.  I think the long delay on this is really my fault for having
> raised an incorrect objection initially --- apologies for that.

Thanks for committing!

Best regards,
Etsuro Fujita

  19. Re: [HACKERS] Useless code in ExecInitModifyTable

    Amit Khandekar <amitdkhan.pg@gmail.com> — 2018-01-19T09:50:23Z

    FYI ...

For the pending update-partition-key patch, we would again require the
rel variable for UPDATE. So in the rebased update-partition-key patch
[1], 'rel' is assigned the root partitioned table. But this time, I
have used the already opened node->rootResultRelInfo to get the root
partitioned table, rather than opening it again. Details : [1] . Sorry
for not noticing this potential conflict earlier. Comments welcome.

[1] : https://www.postgresql.org/message-id/CAJ3gD9cpyM1L0vTrXzrggR%3Dt6MSZtuy_kge1kagMBi0TSKa_UQ%40mail.gmail.com

Thanks
-Amit Khandekar

On 18 January 2018 at 12:48, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote:
> (2018/01/18 4:46), Tom Lane wrote:
>>
>> Pushed.  I think the long delay on this is really my fault for having
>> raised an incorrect objection initially --- apologies for that.
>
>
> Thanks for committing!
>
> Best regards,
> Etsuro Fujita
>

  20. Re: [HACKERS] Useless code in ExecInitModifyTable

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-19T10:14:14Z

    On 2018/01/19 18:50, Amit Khandekar wrote:
> FYI ...
> 
> For the pending update-partition-key patch, we would again require the
> rel variable for UPDATE. So in the rebased update-partition-key patch
> [1], 'rel' is assigned the root partitioned table. But this time, I
> have used the already opened node->rootResultRelInfo to get the root
> partitioned table, rather than opening it again. Details : [1] . Sorry
> for not noticing this potential conflict earlier. Comments welcome.
> 
> [1] : https://www.postgresql.org/message-id/CAJ3gD9cpyM1L0vTrXzrggR%3Dt6MSZtuy_kge1kagMBi0TSKa_UQ%40mail.gmail.com

That's nice.  Actually, the rootResultRelInfo field was introduced [1]
after partitioned_rels [2] and the code that got removed with the patch
that was committed should have gone much earlier.  That is, when
rootResultRelInfo was introduced, but then again as Fujita-san pointed
out, there wasn't much point then (and now) to finding the root table's
Relation pointer in some special place.  But now we need to, for the
update tuple routing case as you said.

Thanks,
Amit

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e180c8aa8ca

[2]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d3cc37f1d801