Re: [HACKERS] Update comment in ExecPartitionCheck

[Etsuro Fujita]

On 2017/08/26 2:28, Robert Haas wrote:

On Tue, Jul 4, 2017 at 4:55 AM, Etsuro Fujita
 wrote:

This comment in an error handling in ExecPartitionCheck():

 if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
 {
 char   *val_desc;
 Relationorig_rel = rel;

 /* See the comment above. */
 if (resultRelInfo->ri_PartitionRoot)

should be updated because we don't have any comment on that above in the
code.  Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this: "See
the comment in ExecConstraints().".  Attached is a patch for that.


Hrm.  I'm not sure I understand which comment in ExecConstraints()
this is supposed to refer to.  Maybe we need to think a bit harder
about how to make this clear.


The comment in ExecConstraints is this:

/*
 * If the tuple has been routed, it's been converted to the
 * partition's rowtype, which might differ from the root
 * table's.  We must convert it back to the root table's
 * rowtype so that val_desc shown error message matches the
 * input tuple.
 */
if (resultRelInfo->ri_PartitionRoot)

How about replacing the comment "See the comment above." in 
ExecPartitionCheck with something like this: "If the tuple has been 
routed, convert it from the partition's rowtype to the root table's. See 
the comment in ExecConstraints().".  I think that would make it easy to 
specify that comment in ExecConstrains.  I'd like to propose to update 
the same comments in other places as well, just for consistency.


PFA an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 1884,1890  ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
char   *val_desc;
Relationorig_rel = rel;
  
!   /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = ExecFetchSlotTuple(slot);
--- 1884,1893 
char   *val_desc;
Relationorig_rel = rel;
  
!   /*
!* If the tuple has been routed, convert it from the partition's
!* rowtype to the root table's.  See the comment in 
ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = ExecFetchSlotTuple(slot);
***
*** 2011,2017  ExecConstraints(ResultRelInfo *resultRelInfo,
char   *val_desc;
Relationorig_rel = rel;
  
!   /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
--- 2014,2023 
char   *val_desc;
Relationorig_rel = rel;
  
!   /*
!* If the tuple has been routed, convert it from the 
partition's
!* rowtype to the root table's.  See the comment above.
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
***
*** 2121,2127  ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
 * USING policy.
 */
case WCO_VIEW_CHECK:
!   /* See the comment in 
ExecConstraints(). */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
--- 2127,2137 
 * USING policy.
 */
case WCO_VIEW_CHECK:
!   /*
!* If the tuple has been routed, 
convert it from the
!* partition's rowtype to the root 
table's.  See the
!* comment in ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = 
ExecFetchSlotTuple(slot);

-- 
Sent via pgsql-hackers 

Re: [HACKERS] Update comment in ExecPartitionCheck

[Robert Haas]

On Tue, Jul 4, 2017 at 4:55 AM, Etsuro Fujita
 wrote:
> This comment in an error handling in ExecPartitionCheck():
>
> if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
> {
> char   *val_desc;
> Relationorig_rel = rel;
>
> /* See the comment above. */
> if (resultRelInfo->ri_PartitionRoot)
>
> should be updated because we don't have any comment on that above in the
> code.  Since we have a comment on that in ExecConstraints() defined just
> below that function, I think the comment should be something like this: "See
> the comment in ExecConstraints().".  Attached is a patch for that.

Hrm.  I'm not sure I understand which comment in ExecConstraints()
this is supposed to refer to.  Maybe we need to think a bit harder
about how to make this clear.

-- 
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] Update comment in ExecPartitionCheck

[Etsuro Fujita]

On 2017/07/04 18:15, Amit Langote wrote:

On 2017/07/04 17:55, Etsuro Fujita wrote:

This comment in an error handling in ExecPartitionCheck():

 if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
 {
 char   *val_desc;
 Relationorig_rel = rel;

 /* See the comment above. */
 if (resultRelInfo->ri_PartitionRoot)

should be updated because we don't have any comment on that above in the
code.  Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this:
"See the comment in ExecConstraints().".  Attached is a patch for that.


Thanks for fixing that.  I forgot to do the same in the patch that got
committed as 15ce775faa428 [1], which moved that code block from
ExecConstraints() to ExecPartitionCheck().


Thanks for the explanation!

In relation to this, I found odd behavior in the error handling.  Since 
that is another topic, I'll start a new thread.


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

Re: [HACKERS] Update comment in ExecPartitionCheck

[Amit Langote]

On 2017/07/04 17:55, Etsuro Fujita wrote:
> This comment in an error handling in ExecPartitionCheck():
> 
> if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
> {
> char   *val_desc;
> Relationorig_rel = rel;
> 
> /* See the comment above. */
> if (resultRelInfo->ri_PartitionRoot)
> 
> should be updated because we don't have any comment on that above in the
> code.  Since we have a comment on that in ExecConstraints() defined just
> below that function, I think the comment should be something like this:
> "See the comment in ExecConstraints().".  Attached is a patch for that.

Thanks for fixing that.  I forgot to do the same in the patch that got
committed as 15ce775faa428 [1], which moved that code block from
ExecConstraints() to ExecPartitionCheck().

Thanks,
Amit

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=15ce775faa428



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

[HACKERS] Update comment in ExecPartitionCheck

[Etsuro Fujita]

This comment in an error handling in ExecPartitionCheck():

if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
{
char   *val_desc;
Relationorig_rel = rel;

/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)

should be updated because we don't have any comment on that above in the 
code.  Since we have a comment on that in ExecConstraints() defined just 
below that function, I think the comment should be something like this: 
"See the comment in ExecConstraints().".  Attached is a patch for that.


Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0f08283..dcf685a 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1864,7 +1864,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
char   *val_desc;
Relationorig_rel = rel;
 
-   /* See the comment above. */
+   /* See the comment in ExecConstraints(). */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple   tuple = ExecFetchSlotTuple(slot);

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