Update comments in nodeModifyTable.c

Started by Etsuro Fujitaover 8 years ago11 messages

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

1 attachment(s)

While working on [1]/messages/by-id/a31f779e-9cb8-1ea5-71a6-bca6adbfa6a4@lab.ntt.co.jp, 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]: /messages/by-id/a31f779e-9cb8-1ea5-71a6-bca6adbfa6a4@lab.ntt.co.jp
/messages/by-id/a31f779e-9cb8-1ea5-71a6-bca6adbfa6a4@lab.ntt.co.jp

Attachments:

nodeModifyTable-comment-update.patchtext/plain; charset=UTF-8; name=nodeModifyTable-comment-update.patchDownload

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");

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Etsuro Fujita (#1)

Re: Update comments in nodeModifyTable.c

On Mon, Jun 5, 2017 at 4:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

In reply to: Robert Haas (#2)

1 attachment(s)

Re: Update comments in nodeModifyTable.c

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

On Mon, Jun 5, 2017 at 4:45 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Attachments:

nodeModifyTable-comment-update-v2.patchtext/plain; charset=UTF-8; name=nodeModifyTable-comment-update-v2.patchDownload

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");

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Etsuro Fujita (#3)

Re: Update comments in nodeModifyTable.c

On Wed, Jun 14, 2017 at 10:40 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

In reply to: Robert Haas (#4)

Re: Update comments in nodeModifyTable.c

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

On Wed, Jun 14, 2017 at 10:40 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Etsuro Fujita (#5)

Re: Update comments in nodeModifyTable.c

On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

In reply to: Robert Haas (#6)

Re: Update comments in nodeModifyTable.c

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

On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Etsuro Fujita (#7)

Re: Update comments in nodeModifyTable.c

On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

In reply to: Robert Haas (#8)

1 attachment(s)

Re: Update comments in nodeModifyTable.c

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

On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

Attachments:

nodeModifyTable-comment-update-v3.patchtext/plain; charset=UTF-8; name=nodeModifyTable-comment-update-v3.patchDownload

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");

#10

Robert Haas

robertmhaas@gmail.com

over 8 years ago

In reply to: Etsuro Fujita (#9)

Re: Update comments in nodeModifyTable.c

On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

#11

Etsuro Fujita

fujita.etsuro@lab.ntt.co.jp

over 8 years ago

In reply to: Robert Haas (#10)

Re: Update comments in nodeModifyTable.c

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

On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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