Обсуждение: [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
Вложения
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
Вложения
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
Вложения
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