Thread: [HACKERS] Useless code in ExecInitModifyTable
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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
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
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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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
(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
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
Attachment
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
(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
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
(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
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 >
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