Re: [HACKERS] Update comments in nodeModifyTable.c

[Etsuro Fujita]

On 2017/08/04 1:52, Robert Haas wrote:

On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita
 wrote:

I updated the patch that way.  Attached is a new version of the patch.


Committed.


Thanks again.

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 comments in nodeModifyTable.c

[Robert Haas]

On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita
 wrote:
> I updated the patch that way.  Attached is a new version of the patch.

Committed.

-- 
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 comments in nodeModifyTable.c

[Etsuro Fujita]

On 2017/08/02 4:07, Robert Haas wrote:

On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
 wrote:

Maybe I'm missing something, but I'm not sure that's a good idea because the
change says like we might have 'wholerow' only for the FDW case, but that
isn't correct because we would have 'wholerow' for a view as well. ISTM that
views should be one of the typical cases, so I'd like to propose to modify
the sentence starting with 'Typically' to something like this: "Typically,
this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign
data wrapper it might be a set of junk attributes sufficient to identify the
remote row."  What do you think about that?


Works for me.


I updated the patch that way.  Attached is a new version of the patch.

Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index 0dde0ed..0199c9d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1696,7 +1696,7 @@ ExecModifyTable(PlanState *pstate)
 * the old relation tuple.
 *
 * Foreign table updates have a wholerow 
attribute when the
-* relation has an AFTER ROW trigger.  Note 
that the wholerow
+* relation has a row-level trigger.  Note that 
the wholerow
 * attribute does not carry system columns.  
Foreign table
 * triggers miss seeing those, except that we 
know enough here
 * to set t_tableOid.  Quite separately from 
this, the FDW may
@@ -2182,8 +2182,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
/*
 * Initialize the junk filter(s) if needed.  INSERT queries need a 
filter
 * if there are any junk attrs in the tlist.  UPDATE and DELETE always
-* need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* need a filter, since there's always at least one junk attribute 
present
+* --- no need to look first.  Typically, this will be a 'ctid' or
+* 'wholerow' attribute, but in the case of a foreign data wrapper it
+* might be a set of junk attributes sufficient to identify the remote
+* row.
 *
 * If there are multiple result relations, each one needs its own junk
 * filter.  Note multiple rels are only possible for UPDATE/DELETE, so 
we
@@ -2251,7 +2254,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
else if (relkind == 
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER 
trigger, there should be a
+* When there is a row-level 
trigger, there should be a
 * wholerow attribute.
 */
j->jf_junkAttNo = 
ExecFindJunkAttribute(j, "wholerow");

-- 
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 comments in nodeModifyTable.c

[Robert Haas]

On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
 wrote:
> Maybe I'm missing something, but I'm not sure that's a good idea because the
> change says like we might have 'wholerow' only for the FDW case, but that
> isn't correct because we would have 'wholerow' for a view as well. ISTM that
> views should be one of the typical cases, so I'd like to propose to modify
> the sentence starting with 'Typically' to something like this: "Typically,
> this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign
> data wrapper it might be a set of junk attributes sufficient to identify the
> remote row."  What do you think about that?

Works for me.

-- 
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 comments in nodeModifyTable.c

[Etsuro Fujita]

On 2017/08/01 1:03, Robert Haas wrote:

On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita
 wrote:

On 2017/07/26 22:39, Robert Haas wrote:

So the first part of the change weakens the assertion that a 'ctid' or
'wholerow' attribute will always be present by saying that an FDW may
instead have other attributes sufficient to identify the row.


That's right.


But
then the additional sentence says that there will be a 'wholerow'
attribute after all.  So this whole change seems to me to be going
around in a circle.


What I mean by the additional one is: if the result table that is a foreign
table has a row-level UPDATE/DELETE trigger, a 'wholerow' will also be
present.  So, if the result table didn't have the trigger, there wouldn't be
'whole-row', so in that case it could be possible that there would be only
attributes other than 'ctid' and 'wholerow'.


I think maybe something like this would be clearer, then:

  /*
   * Initialize the junk filter(s) if needed.  INSERT queries need a filter
   * if there are any junk attrs in the tlist.  UPDATE and DELETE always
- * need a filter, since there's always a junk 'ctid' or 'wholerow'
- * attribute present --- no need to look first.
+ * need a filter, since there's always at least one junk attribute present
+ * --- no need to look first.  Typically, this will be a 'ctid'
+ * attribute, but in the case of a foreign data wrapper it might be a
+ * 'wholerow' attribute or some other set of junk attributes sufficient to
+ * identify the remote row.
   *
   * If there are multiple result relations, each one needs its own junk
   * filter.  Note multiple rels are only possible for UPDATE/DELETE, so we


Maybe I'm missing something, but I'm not sure that's a good idea because 
the change says like we might have 'wholerow' only for the FDW case, but 
that isn't correct because we would have 'wholerow' for a view as well. 
ISTM that views should be one of the typical cases, so I'd like to 
propose to modify the sentence starting with 'Typically' to something 
like this: "Typically, this will be a 'ctid' or 'wholerow' attribute, 
but in the case of a foreign data wrapper it might be a set of junk 
attributes sufficient to identify the remote row."  What do you think 
about 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

Re: [HACKERS] Update comments in nodeModifyTable.c

[Robert Haas]

On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita
 wrote:
> On 2017/07/26 22:39, Robert Haas wrote:
>> So the first part of the change weakens the assertion that a 'ctid' or
>> 'wholerow' attribute will always be present by saying that an FDW may
>> instead have other attributes sufficient to identify the row.
>
> That's right.
>
>> But
>> then the additional sentence says that there will be a 'wholerow'
>> attribute after all.  So this whole change seems to me to be going
>> around in a circle.
>
> What I mean by the additional one is: if the result table that is a foreign
> table has a row-level UPDATE/DELETE trigger, a 'wholerow' will also be
> present.  So, if the result table didn't have the trigger, there wouldn't be
> 'whole-row', so in that case it could be possible that there would be only
> attributes other than 'ctid' and 'wholerow'.

I think maybe something like this would be clearer, then:

 /*
  * Initialize the junk filter(s) if needed.  INSERT queries need a filter
  * if there are any junk attrs in the tlist.  UPDATE and DELETE always
- * need a filter, since there's always a junk 'ctid' or 'wholerow'
- * attribute present --- no need to look first.
+ * need a filter, since there's always at least one junk attribute present
+ * --- no need to look first.  Typically, this will be a 'ctid'
+ * attribute, but in the case of a foreign data wrapper it might be a
+ * 'wholerow' attribute or some other set of junk attributes sufficient to
+ * identify the remote row.
  *
  * If there are multiple result relations, each one needs its own junk
  * filter.  Note multiple rels are only possible for UPDATE/DELETE, so we

-- 
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 comments in nodeModifyTable.c

[Etsuro Fujita]

On 2017/07/26 22:39, Robert Haas wrote:

On Wed, Jun 14, 2017 at 10:40 PM, Etsuro Fujita
 wrote:

Attached is an updated version of the patch.


Well, now I'm confused:

   * Initialize the junk filter(s) if needed.  INSERT queries need a filter
   * if there are any junk attrs in the tlist.  UPDATE and DELETE always
   * need a filter, since there's always a junk 'ctid' or 'wholerow'
- * attribute present --- no need to look first.
+ * attribute present if not foreign table, and if foreign table, there
+ * are always junk attributes present the FDW needs to identify the exact
+ * row to update or delete --- no need to look first.  For foreign tables,
+ * there's also a wholerow attribute when the relation has a row-level
+ * trigger on UPDATE/DELETE but not on INSERT.

So the first part of the change weakens the assertion that a 'ctid' or
'wholerow' attribute will always be present by saying that an FDW may
instead have other attributes sufficient to identify the row.


That's right.


But
then the additional sentence says that there will be a 'wholerow'
attribute after all.  So this whole change seems to me to be going
around in a circle.


What I mean by the additional one is: if the result table that is a 
foreign table has a row-level UPDATE/DELETE trigger, a 'wholerow' will 
also be present.  So, if the result table didn't have the trigger, there 
wouldn't be 'whole-row', so in that case it could be possible that there 
would be only attributes other than 'ctid' and 'wholerow'.


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 comments in nodeModifyTable.c

[Robert Haas]

On Wed, Jun 14, 2017 at 10:40 PM, Etsuro Fujita
 wrote:
> Agreed, but I think it's better to add that detail to this comment in
> ExecInitModifyTable:
>
>  * Initialize the junk filter(s) if needed.  INSERT queries need a
> filter
>  * if there are any junk attrs in the tlist.  UPDATE and DELETE always
>  * need a filter, since there's always a junk 'ctid' or 'wholerow'
>  * attribute present --- no need to look first.
>
> I'd also like to propose to update the third sentence in this comment, since
> there isn't necessarily a ctid or wholerow in the UPDATE/DELETE tlist when
> the result relation is a foreign table.
>
> Attached is an updated version of the patch.

Well, now I'm confused:

  * Initialize the junk filter(s) if needed.  INSERT queries need a filter
  * if there are any junk attrs in the tlist.  UPDATE and DELETE always
  * need a filter, since there's always a junk 'ctid' or 'wholerow'
- * attribute present --- no need to look first.
+ * attribute present if not foreign table, and if foreign table, there
+ * are always junk attributes present the FDW needs to identify the exact
+ * row to update or delete --- no need to look first.  For foreign tables,
+ * there's also a wholerow attribute when the relation has a row-level
+ * trigger on UPDATE/DELETE but not on INSERT.

So the first part of the change weakens the assertion that a 'ctid' or
'wholerow' attribute will always be present by saying that an FDW may
instead have other attributes sufficient to identify the row.  But
then the additional sentence says that there will be a 'wholerow'
attribute after all.  So this whole change seems to me to be going
around in a circle.

-- 
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 comments in nodeModifyTable.c

[Etsuro Fujita]

On 2017/06/07 0:30, Robert Haas wrote:

On Mon, Jun 5, 2017 at 4:45 AM, Etsuro Fujita
 wrote:

While working on [1], I noticed that the comment in ExecModifyTable:

  * Foreign table updates have a wholerow attribute when the
  * relation has an AFTER ROW trigger.

is not 100% correct because a foreign table has a wholerow attrubute when
the relation has an AFTER ROW or BEFORE ROW trigger (see
rewriteTargetListUD).  So I'd propose s/an AFTER ROW trigger/a row-level
trigger/.  Attached is a patch for that.


That seems better, but looking at rewriteTargetListUD, it seems that
the actual rule is that this happens when there is a row-level on
either UPDATE or DELETE.  If there is only a row-level trigger on
INSERT, then it is not done.  Perhaps we should try to include that
detail in the comment as well.


Agreed, but I think it's better to add that detail to this comment in 
ExecInitModifyTable:


 * Initialize the junk filter(s) if needed.  INSERT queries need a 
filter

 * if there are any junk attrs in the tlist.  UPDATE and DELETE always
 * need a filter, since there's always a junk 'ctid' or 'wholerow'
 * attribute present --- no need to look first.

I'd also like to propose to update the third sentence in this comment, 
since there isn't necessarily a ctid or wholerow in the UPDATE/DELETE 
tlist when the result relation is a foreign table.


Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index cf555fe..07bc870 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1554,7 +1554,7 @@ ExecModifyTable(ModifyTableState *node)
 * the old relation tuple.
 *
 * Foreign table updates have a wholerow 
attribute when the
-* relation has an AFTER ROW trigger.  Note 
that the wholerow
+* relation has a row-level trigger.  Note that 
the wholerow
 * attribute does not carry system columns.  
Foreign table
 * triggers miss seeing those, except that we 
know enough here
 * to set t_tableOid.  Quite separately from 
this, the FDW may
@@ -2025,7 +2025,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
 * Initialize the junk filter(s) if needed.  INSERT queries need a 
filter
 * if there are any junk attrs in the tlist.  UPDATE and DELETE always
 * need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* attribute present if not foreign table, and if foreign table, there
+* are always junk attributes present the FDW needs to identify the 
exact
+* row to update or delete --- no need to look first.  For foreign 
tables,
+* there's also a wholerow attribute when the relation has a row-level
+* trigger on UPDATE/DELETE but not on INSERT.
 *
 * If there are multiple result relations, each one needs its own junk
 * filter.  Note multiple rels are only possible for UPDATE/DELETE, so 
we
@@ -2093,7 +2097,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
else if (relkind == 
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER 
trigger, there should be a
+* When there is a row-level 
trigger, there should be a
 * wholerow attribute.
 */
j->jf_junkAttNo = 
ExecFindJunkAttribute(j, "wholerow");

-- 
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 comments in nodeModifyTable.c

[Robert Haas]

On Mon, Jun 5, 2017 at 4:45 AM, Etsuro Fujita
 wrote:
> While working on [1], I noticed that the comment in ExecModifyTable:
>
>  * Foreign table updates have a wholerow attribute when the
>  * relation has an AFTER ROW trigger.
>
> is not 100% correct because a foreign table has a wholerow attrubute when
> the relation has an AFTER ROW or BEFORE ROW trigger (see
> rewriteTargetListUD).  So I'd propose s/an AFTER ROW trigger/a row-level
> trigger/.  Attached is a patch for that.

That seems better, but looking at rewriteTargetListUD, it seems that
the actual rule is that this happens when there is a row-level on
either UPDATE or DELETE.  If there is only a row-level trigger on
INSERT, then it is not done.  Perhaps we should try to include that
detail in the comment as well.

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

[HACKERS] Update comments in nodeModifyTable.c

[Etsuro Fujita]

While working on [1], I noticed that the comment in ExecModifyTable:

 * Foreign table updates have a wholerow attribute when the
 * relation has an AFTER ROW trigger.

is not 100% correct because a foreign table has a wholerow attrubute 
when the relation has an AFTER ROW or BEFORE ROW trigger (see 
rewriteTargetListUD).  So I'd propose s/an AFTER ROW trigger/a row-level 
trigger/.  Attached is a patch for that.


Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/a31f779e-9cb8-1ea5-71a6-bca6adbfa6a4%40lab.ntt.co.jp
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index cf555fe..5dde93c 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1554,7 +1554,7 @@ ExecModifyTable(ModifyTableState *node)
 * the old relation tuple.
 *
 * Foreign table updates have a wholerow 
attribute when the
-* relation has an AFTER ROW trigger.  Note 
that the wholerow
+* relation has a row-level trigger.  Note that 
the wholerow
 * attribute does not carry system columns.  
Foreign table
 * triggers miss seeing those, except that we 
know enough here
 * to set t_tableOid.  Quite separately from 
this, the FDW may
@@ -2093,7 +2093,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, 
int eflags)
else if (relkind == 
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER 
trigger, there should be a
+* When there is a row-level 
trigger, there should be a
 * wholerow attribute.
 */
j->jf_junkAttNo = 
ExecFindJunkAttribute(j, "wholerow");

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