Re: [HACKERS] Useless code in ExecInitModifyTable

[Ryan Murphy]

> 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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Tom Lane]

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

> 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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Ryan Murphy]

Hello!

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

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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Etsuro Fujita]

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
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 45,51 
  #include "foreign/fdwapi.h"
  #include "miscadmin.h"
  #include "nodes/nodeFuncs.h"
- #include "parser/parsetree.h"
  #include "storage/bufmgr.h"
  #include "storage/lmgr.h"
  #include "utils/builtins.h"
--- 45,50 
***
*** 1894,1913  ExecInitModifyTable(ModifyTable *node, EState *estate, int 
eflags)
  
estate->es_result_relation_info = saved_resultRelInfo;
  
-   /* 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 */
-   }
-   else
-   rel = mtstate->resultRelInfo->ri_RelationDesc;
- 
/* Build state for INSERT tuple routing */
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
--- 1893,1900 
  
estate->es_result_relation_info = saved_resultRelInfo;
  
/* Build state for INSERT tuple routing */
+   rel = mtstate->resultRelInfo->ri_RelationDesc;
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
***
*** 2091,2100  ExecInitModifyTable(ModifyTable *node, EState *estate, int 
eflags)
mtstate->ps.ps_ExprContext = NULL;
}
  
-   /* Close the root partitioned rel if we opened it above. */
-   if (rel != mtstate->resultRelInfo->ri_RelationDesc)
-   heap_close(rel, NoLock);
- 
/*
 * If needed, Initialize target list, projection and qual for ON 
CONFLICT
 * DO UPDATE.
--- 2078,2083 

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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Amit Langote]

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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Etsuro Fujita]

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




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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Tom Lane]

Robert Haas  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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Robert Haas]

On Wed, Jun 21, 2017 at 10:33 AM, Tom Lane  wrote:
> Amit Langote  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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Tom Lane]

Amit Langote  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

Re: [HACKERS] Useless code in ExecInitModifyTable

[Amit Langote]

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

[HACKERS] Useless code in ExecInitModifyTable

[Etsuro Fujita]

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
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 45,51 
  #include "foreign/fdwapi.h"
  #include "miscadmin.h"
  #include "nodes/nodeFuncs.h"
- #include "parser/parsetree.h"
  #include "storage/bufmgr.h"
  #include "storage/lmgr.h"
  #include "utils/builtins.h"
--- 45,50 
***
*** 1767,1786  ExecInitModifyTable(ModifyTable *node, EState *estate, int 
eflags)
  
estate->es_result_relation_info = saved_resultRelInfo;
  
-   /* 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 */
-   }
-   else
-   rel = mtstate->resultRelInfo->ri_RelationDesc;
- 
/* Build state for INSERT tuple routing */
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
--- 1766,1773 
  
estate->es_result_relation_info = saved_resultRelInfo;
  
/* Build state for INSERT tuple routing */
+   rel = mtstate->resultRelInfo->ri_RelationDesc;
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
***
*** 1959,1968  ExecInitModifyTable(ModifyTable *node, EState *estate, int 
eflags)
mtstate->ps.ps_ExprContext = NULL;
}
  
-   /* Close the root partitioned rel if we opened it above. */
-   if (rel != mtstate->resultRelInfo->ri_RelationDesc)
-   heap_close(rel, NoLock);
- 
/*
 * If needed, Initialize target list, projection and qual for ON 
CONFLICT
 * DO UPDATE.
--- 1946,1951 

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