Useless code in ExecInitModifyTable

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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]/messages/by-id/e87c4a6d-23d7-5e7c-e8db-44ed418eb5d1@lab.ntt.co.jp.

Best regards,
Etsuro Fujita

[1]: /messages/by-id/e87c4a6d-23d7-5e7c-e8db-44ed418eb5d1@lab.ntt.co.jp
/messages/by-id/e87c4a6d-23d7-5e7c-e8db-44ed418eb5d1@lab.ntt.co.jp

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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]: /messages/by-id/CAJ3gD9cpyM1L0vTrXzrggR=t6MSZtuy_kge1kagMBi0TSKa_UQ@mail.gmail.com
have used the already opened node->rootResultRelInfo to get the root
partitioned table, rather than opening it again. Details : [1]/messages/by-id/CAJ3gD9cpyM1L0vTrXzrggR=t6MSZtuy_kge1kagMBi0TSKa_UQ@mail.gmail.com . Sorry
for not noticing this potential conflict earlier. Comments welcome.

[1]: /messages/by-id/CAJ3gD9cpyM1L0vTrXzrggR=t6MSZtuy_kge1kagMBi0TSKa_UQ@mail.gmail.com

Thanks
-Amit Khandekar

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] : /messages/by-id/CAJ3gD9cpyM1L0vTrXzrggR=t6MSZtuy_kge1kagMBi0TSKa_UQ@mail.gmail.com

That's nice. Actually, the rootResultRelInfo field was introduced [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e180c8aa8ca
after partitioned_rels [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d3cc37f1d801 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
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
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=d3cc37f1d801