Thread: [HACKERS] Update comments in nodeModifyTable.c
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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 inthe 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
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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