On 2021/04/22 20:27, Bharath Rupireddy wrote:
> On Thu, Apr 22, 2021 at 12:06 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>> On 2021/04/22 9:39, Bharath Rupireddy wrote:
>>> One comment on truncate_foreign_table_docs_v1.patch:
>>> 1) I think it is "to be truncated"
>>> + <literal>rels</literal> is a list of <structname>Relation</structname>
>>> + data structures for each foreign table to truncated.
>>
>> Fixed. Thanks!
>>
>>> How about a slightly changed phrasing like below?
>>> + <literal>rels</literal> is a list of <structname>Relation</structname>
>>> + data structures of foreign tables to truncate.
>> Either works at least for me. If you think that this phrasing is
>> more precise or better, I'm ok with that and will update the patch again.
>
> IMO, "rels is a list of Relation data structures of foreign tables to
> truncate." looks better.
Fixed.
Thanks for reviewing the patches.
Attached are the updated versions of the patches.
These patches include the fixes pointed by Justin.
>
>>>>> 3) How about adding an extra para(after below para in
>>>>> postgres_fdw.sgml) on WHY we don't push "ONLY" to foreign tables while
>>>>> truncating? We could add to the same para for other options if at all
>>>>> we don't choose to push them.
>>>>> <command>DELETE</command>, or <command>TRUNCATE</command>.
>>>>> (Of course, the remote user you have specified in your user mapping must
>>>>> have privileges to do these things.)
>>>>
>>>> I agree to document the behavior that ONLY option is always ignored
>>>> for foreign tables. But I'm not sure if we can document WHY.
>>>> Because I could not find the past discussion about why ONLY option is
>>>> ignored on SELECT, etc... Maybe it's enough to document the behavior?
>>>
>>> +1 to specify in the documentation about ONLY option is always
>>> ignored.
>>
>> Added.
>>
>>> But can we specify the WHY part within deparseTruncateSql, it
>>> will be there for developer reference? I feel it's better if this
>>> change goes with truncate_foreign_table_dont_pass_only_clause_v2.patch
>>
>> I added this information into fdwhandler.sgml because the developers
>> usually read fdwhandler.sgml.
>
> Thanks!
>
> + <para>
> + Note that information about <literal>ONLY</literal> options specified
> + in the original <command>TRUNCATE</command> command is not passed to
>
> I think it is not "information about", no? We just don't pass ONLY
> option instead we skip it. IMO, we can say "Note that the ONLY option
> specified with a foreign table in the original TRUNCATE command is
> skipped and not passed to ExecForeignTruncate."
Probably I still fail to understand your point.
But if "information about" is confusing, I'm ok to
remove that. Fixed.
>
> + <function>ExecForeignTruncate</function>. This is the same behavior as
> + for the callback functions for <command>SELECT</command>,
> + <command>UPDATE</command> and <command>DELETE</command> on
> + a foreign table.
>
> How about "This behaviour is similar to the callback functions of
> SELECT, UPDATE, DELETE on a foreign table"?
Fixed.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
