Thread: TRUNCATE on foreign table

IIUC, "truncatable" would be set to "false" for relations which do not
have physical storage e.g. views but will be true for regular tables.
When we are importing schema we need to set "truncatable"
appropriately. Is that something we will support with this patch?

Why would one want to truncate a foreign table instead of truncating
actual table wherever it is?

On Sun, Feb 7, 2021 at 6:06 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> Thank you for your comment! :D
> I have fixed it and attached the revised patch.
>
> regards,
>
>
>
> 2021年2月7日(日) 2:08 Zhihong Yu <zyu@yugabyte.com>:
>>
>> Hi,
>> +               if (strcmp(defel->defname, "truncatable") == 0)
>> +                   server_truncatable = defGetBoolean(defel);
>>
>> Looks like we can break out of the loop when the condition is met.
>>
>> +           /* ExecForeignTruncate() is invoked for each server */
>>
>> The method name in the comment is slightly different from the actual method name.
>>
>> +           if (strcmp(defel->defname, "truncatable") == 0)
>> +               truncatable = defGetBoolean(defel);
>>
>> We can break out of the loop when the condition is met.
>>
>> Cheers
>>
>> On Sat, Feb 6, 2021 at 5:11 AM Kazutaka Onishi <onishi@heterodb.com> wrote:
>>>
>>> Hello,
>>>
>>> The attached patch is for supporting "TRUNCATE" on  foreign tables.
>>>
>>> This patch includes:
>>> * Adding "ExecForeignTruncate" function into FdwRoutine.
>>> * Enabling "postgres_fdw" to use TRUNCATE.
>>>
>>> This patch was proposed by Kaigai-san in March 2020,
>>> but it was returned because it can't be applied to the latest source codes.
>>>
>>> Please refer to the discussion.
>>>
https://www.postgresql.org/message-id/flat/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com#3b6c6ff85ff5c722b36c7a09b2dd7165
>>>
>>> I have fixed the patch due to submit it to Commit Fest 2021-03.
>>>
>>> regards,
>>>
>>> --
>>> ------------------
>>> Kazutaka Onishi
>>> (onishi@heterodb.com)
>
>
>
> --
> ------------------
> Kazutaka Onishi
> (onishi@heterodb.com)
>
>
> --
> ------------------
> Kazutaka Onishi
> (onishi@heterodb.com)



--
Best Wishes,
Ashutosh Bapat

On Tue, Feb 9, 2021 at 5:31 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> Why would one want to truncate a foreign table instead of truncating
> actual table wherever it is?

I think when the deletion on foreign tables (which actually deletes
rows from the remote table?) is allowed, it does make sense to have a
way to truncate the remote table via foreign table. Also, it can avoid
going to each and every remote server and doing the truncation
instead.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Feb 9, 2021 at 5:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Tue, Feb 9, 2021 at 5:31 PM Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> > Why would one want to truncate a foreign table instead of truncating
> > actual table wherever it is?
>
> I think when the deletion on foreign tables (which actually deletes
> rows from the remote table?) is allowed, it does make sense to have a
> way to truncate the remote table via foreign table. Also, it can avoid
> going to each and every remote server and doing the truncation
> instead.

DELETE is very different from TRUNCATE. Application may want to DELETE
based on a join with a local table and hence it can not be executed on
a foreign server. That's not true with TRUNCATE.

-- 
Best Wishes,
Ashutosh Bapat

On Tue, Feb 9, 2021 at 7:45 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> > IIUC, "truncatable" would be set to "false" for relations which do not
> > have physical storage e.g. views but will be true for regular tables.
>
> "truncatable" option is just for the foreign table and it's not related with whether it's on a physical storage or
not.
> "postgres_fdw" already has "updatable" option to make the table read-only.
> However, "updatable" is for DML, and it's not suitable for TRUNCATE.
> Therefore new options "truncatable" was added.
>
> Please refer to this message for details.
> https://www.postgresql.org/message-id/20200128040346.GC1552%40paquier.xyz
>
> > DELETE is very different from TRUNCATE. Application may want to DELETE
> > based on a join with a local table and hence it can not be executed on
> > a foreign server. That's not true with TRUNCATE.
>
> Yeah, As you say, Applications doesn't need  TRUNCATE.
> We're focusing for analytical use, namely operating huge data.
> TRUNCATE can erase rows faster than DELETE.

The question is why can't that truncate be run on the foreign server
itself rather than local server?



-- 
Best Wishes,
Ashutosh Bapat

2021年2月10日(水) 13:55 Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>:
>
> On Tue, Feb 9, 2021 at 7:45 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> >
> > > IIUC, "truncatable" would be set to "false" for relations which do not
> > > have physical storage e.g. views but will be true for regular tables.
> >
> > "truncatable" option is just for the foreign table and it's not related with whether it's on a physical storage or
not.
> > "postgres_fdw" already has "updatable" option to make the table read-only.
> > However, "updatable" is for DML, and it's not suitable for TRUNCATE.
> > Therefore new options "truncatable" was added.
> >
> > Please refer to this message for details.
> > https://www.postgresql.org/message-id/20200128040346.GC1552%40paquier.xyz
> >
> > > DELETE is very different from TRUNCATE. Application may want to DELETE
> > > based on a join with a local table and hence it can not be executed on
> > > a foreign server. That's not true with TRUNCATE.
> >
> > Yeah, As you say, Applications doesn't need  TRUNCATE.
> > We're focusing for analytical use, namely operating huge data.
> > TRUNCATE can erase rows faster than DELETE.
>
> The question is why can't that truncate be run on the foreign server
> itself rather than local server?
>
At least, PostgreSQL applies different access permissions on TRUNCATE.
If unconditional DELETE implicitly promotes to TRUNCATE, DB administrator
has to allow TRUNCATE permission on the remote table also.

Also, TRUNCATE acquires stronger lock the DELETE.
DELETE still allows concurrent accesses to the table, even though TRUNCATE
takes AccessExclusive lock, thus, FDW driver has to control the
concurrent accesses
by itself, if we have no dedicated TRUNCATE interface.

Thanks,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On Wed, Feb 10, 2021 at 10:58 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> That's because using the foreign server is difficult for the user.
>
> For example, the user doesn't always have the permission to login to the forein server.
> In some cases, the foreign table has been created by the administrator that has permission to access the two servers
andthe user only uses the local server.
 
> Then the user has to ask the administrator to run TRUNCATE every time.

That might actually be seen as a loophole but ...

>
> Furthermore,there are some fdw extensions which don't support SQL. mongo_fdw, redis_fdw, etc...
> These extensions have been used to provide SQL interfaces to the users.
> It's hard for the user to run TRUNCATE after learning each database.

this has some appeal.

Thanks for sharing the usecases.
-- 
Best Wishes,
Ashutosh Bapat

On 2021/03/13 18:57, Kazutaka Onishi wrote:
> I have fixed the patch to pass check-world test. :D

Thanks for updating the patch! Here are some review comments from me.


       By default all foreign tables using <filename>postgres_fdw</filename> are assumed
       to be updatable.  This may be overridden using the following option:

In postgres-fdw.sgml, "and truncatable" should be appended into
the above first description? Also "option" in the second description
should be a plural form "options"?


      <command>TRUNCATE</command> is not currently supported for foreign tables.
      This implies that if a specified table has any descendant tables that are
      foreign, the command will fail.

truncate.sgml should be updated because, for example, it contains
the above descriptions.


+     <literal>frels_extra</literal> is same length with
+     <literal>frels_list</literal>, that delivers extra information of
+     the context where the foreign-tables are truncated.
+    </para>

Don't we need to document the detail information about frels_extra?
Otherwise the developers of FDW would fail to understand how to
handle the frels_extra when trying to make their FDWs support TRUNCATE.


+        relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
+                relids_extra = lappend_int(relids_extra, -1);

postgres_fdw determines whether to specify ONLY or not by checking
whether the passed extra value is zero or not. That is, for example,
using only 0 and 1 for extra values is enough for the purpose. But
ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
these three values necessary?


With the patch, if both local and foreign tables are specified as
the target tables to truncate, TRUNCATE command tries to truncate
foreign tables after truncating local ones. That is, if "truncatable"
option is set to false or enough permission to truncate is not granted
yet in the foreign server, an error will be thrown after the local tables
are truncated. I don't think this is good order of processings. IMO,
instead, we should check whether foreign tables can be truncated
before any actual truncation operations. For example, we can easily
do that by truncate foreign tables before local ones. Thought?


XLOG_HEAP_TRUNCATE record is written even for the truncation of
a foreign table. Why is this necessary?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Fujii-san,

Thank you for your review!
Now I prepare v5 patch and I'll answer to your each comment. please
check this again.
m(_ _)m

1. In postgres-fdw.sgml, "and truncatable" should be appended into the
above first description?
2. truncate.sgml should be updated because, for example, it contains
the above descriptions.

Yeah, you're right. I've fixed it.



3.  Don't we need to document the detail information about frels_extra?

I've written about frels_extra into fdwhander.sgml.



4. postgres_fdw determines whether to specify ONLY or not by checking
whether the passed extra value is zero or not.

Please refer this:
https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
> Negative value means that foreign-tables are not specified in the TRUNCATE
> command, but truncated due to dependency (like partition's child leaf).

I've added this information into fdwhandler.sgml.



5. For example, we can easily do that by truncate foreign tables
before local ones. Thought?

Umm... yeah, I feel it's better procedure, but not so required because
TRUNCATE is NOT called frequently.
Certainly, we already have postgresIsForeignUpdatable() to check
whether the foreign table is updatable or not.
Following this way, we have to add postgresIsForeignTruncatable() to check.
However, Unlike UPDATE, TRUNCATE is NOT called frequently. Current
procedure is inefficient but works correctly.
Thus, I feel postgresIsForeignTruncatable() is not needed.


6. XLOG_HEAP_TRUNCATE record is written even for the truncation of a
foreign table. Why is this necessary?

Please give us more time to investigate this.

2021年3月25日(木) 3:47 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
>
>
> On 2021/03/13 18:57, Kazutaka Onishi wrote:
> > I have fixed the patch to pass check-world test. :D
>
> Thanks for updating the patch! Here are some review comments from me.
>
>
>        By default all foreign tables using <filename>postgres_fdw</filename> are assumed
>        to be updatable.  This may be overridden using the following option:
>
> In postgres-fdw.sgml, "and truncatable" should be appended into
> the above first description? Also "option" in the second description
> should be a plural form "options"?
>
>
>       <command>TRUNCATE</command> is not currently supported for foreign tables.
>       This implies that if a specified table has any descendant tables that are
>       foreign, the command will fail.
>
> truncate.sgml should be updated because, for example, it contains
> the above descriptions.
>
>
> +     <literal>frels_extra</literal> is same length with
> +     <literal>frels_list</literal>, that delivers extra information of
> +     the context where the foreign-tables are truncated.
> +    </para>
>
> Don't we need to document the detail information about frels_extra?
> Otherwise the developers of FDW would fail to understand how to
> handle the frels_extra when trying to make their FDWs support TRUNCATE.
>
>
> +               relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
> +                               relids_extra = lappend_int(relids_extra, -1);
>
> postgres_fdw determines whether to specify ONLY or not by checking
> whether the passed extra value is zero or not. That is, for example,
> using only 0 and 1 for extra values is enough for the purpose. But
> ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
> these three values necessary?
>
>
> With the patch, if both local and foreign tables are specified as
> the target tables to truncate, TRUNCATE command tries to truncate
> foreign tables after truncating local ones. That is, if "truncatable"
> option is set to false or enough permission to truncate is not granted
> yet in the foreign server, an error will be thrown after the local tables
> are truncated. I don't think this is good order of processings. IMO,
> instead, we should check whether foreign tables can be truncated
> before any actual truncation operations. For example, we can easily
> do that by truncate foreign tables before local ones. Thought?
>
>
> XLOG_HEAP_TRUNCATE record is written even for the truncation of
> a foreign table. Why is this necessary?
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Onishi-san,

The v5 patch contains full-contents of "src/backend/commands/tablecmds.c.orig".
Please check it.

2021年3月28日(日) 2:37 Kazutaka Onishi <onishi@heterodb.com>:
>
> Fujii-san,
>
> Thank you for your review!
> Now I prepare v5 patch and I'll answer to your each comment. please
> check this again.
> m(_ _)m
>
> 1. In postgres-fdw.sgml, "and truncatable" should be appended into the
> above first description?
> 2. truncate.sgml should be updated because, for example, it contains
> the above descriptions.
>
> Yeah, you're right. I've fixed it.
>
>
>
> 3.  Don't we need to document the detail information about frels_extra?
>
> I've written about frels_extra into fdwhander.sgml.
>
>
>
> 4. postgres_fdw determines whether to specify ONLY or not by checking
> whether the passed extra value is zero or not.
>
> Please refer this:
> https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
> > Negative value means that foreign-tables are not specified in the TRUNCATE
> > command, but truncated due to dependency (like partition's child leaf).
>
> I've added this information into fdwhandler.sgml.
>
>
>
> 5. For example, we can easily do that by truncate foreign tables
> before local ones. Thought?
>
> Umm... yeah, I feel it's better procedure, but not so required because
> TRUNCATE is NOT called frequently.
> Certainly, we already have postgresIsForeignUpdatable() to check
> whether the foreign table is updatable or not.
> Following this way, we have to add postgresIsForeignTruncatable() to check.
> However, Unlike UPDATE, TRUNCATE is NOT called frequently. Current
> procedure is inefficient but works correctly.
> Thus, I feel postgresIsForeignTruncatable() is not needed.
>
>
> 6. XLOG_HEAP_TRUNCATE record is written even for the truncation of a
> foreign table. Why is this necessary?
>
> Please give us more time to investigate this.
>
> 2021年3月25日(木) 3:47 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >
> >
> >
> > On 2021/03/13 18:57, Kazutaka Onishi wrote:
> > > I have fixed the patch to pass check-world test. :D
> >
> > Thanks for updating the patch! Here are some review comments from me.
> >
> >
> >        By default all foreign tables using <filename>postgres_fdw</filename> are assumed
> >        to be updatable.  This may be overridden using the following option:
> >
> > In postgres-fdw.sgml, "and truncatable" should be appended into
> > the above first description? Also "option" in the second description
> > should be a plural form "options"?
> >
> >
> >       <command>TRUNCATE</command> is not currently supported for foreign tables.
> >       This implies that if a specified table has any descendant tables that are
> >       foreign, the command will fail.
> >
> > truncate.sgml should be updated because, for example, it contains
> > the above descriptions.
> >
> >
> > +     <literal>frels_extra</literal> is same length with
> > +     <literal>frels_list</literal>, that delivers extra information of
> > +     the context where the foreign-tables are truncated.
> > +    </para>
> >
> > Don't we need to document the detail information about frels_extra?
> > Otherwise the developers of FDW would fail to understand how to
> > handle the frels_extra when trying to make their FDWs support TRUNCATE.
> >
> >
> > +               relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
> > +                               relids_extra = lappend_int(relids_extra, -1);
> >
> > postgres_fdw determines whether to specify ONLY or not by checking
> > whether the passed extra value is zero or not. That is, for example,
> > using only 0 and 1 for extra values is enough for the purpose. But
> > ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
> > these three values necessary?
> >
> >
> > With the patch, if both local and foreign tables are specified as
> > the target tables to truncate, TRUNCATE command tries to truncate
> > foreign tables after truncating local ones. That is, if "truncatable"
> > option is set to false or enough permission to truncate is not granted
> > yet in the foreign server, an error will be thrown after the local tables
> > are truncated. I don't think this is good order of processings. IMO,
> > instead, we should check whether foreign tables can be truncated
> > before any actual truncation operations. For example, we can easily
> > do that by truncate foreign tables before local ones. Thought?
> >
> >
> > XLOG_HEAP_TRUNCATE record is written even for the truncation of
> > a foreign table. Why is this necessary?
> >
> > Regards,
> >
> > --
> > Fujii Masao
> > Advanced Computing Technology Center
> > Research and Development Headquarters
> > NTT DATA CORPORATION



--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

Fujii-san,

> XLOG_HEAP_TRUNCATE record is written even for the truncation of
> a foreign table. Why is this necessary?
>
Foreign-tables are often used to access local data structure, like
columnar data files
on filesystem, not only remote accesses like postgres_fdw.
In case when we want to implement logical replication on this kind of
foreign-tables,
truncate-command must be delivered to subscriber node - to truncate
its local data.

In case of remote-access FDW drivers, truncate-command on the subscriber-side is
probably waste of cycles, however, only FDW driver and DBA who configured the
foreign-table know whether it is necessary, or not.

How about your opinions?

Best regards,

2021年3月25日(木) 3:47 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
>
>
> On 2021/03/13 18:57, Kazutaka Onishi wrote:
> > I have fixed the patch to pass check-world test. :D
>
> Thanks for updating the patch! Here are some review comments from me.
>
>
>        By default all foreign tables using <filename>postgres_fdw</filename> are assumed
>        to be updatable.  This may be overridden using the following option:
>
> In postgres-fdw.sgml, "and truncatable" should be appended into
> the above first description? Also "option" in the second description
> should be a plural form "options"?
>
>
>       <command>TRUNCATE</command> is not currently supported for foreign tables.
>       This implies that if a specified table has any descendant tables that are
>       foreign, the command will fail.
>
> truncate.sgml should be updated because, for example, it contains
> the above descriptions.
>
>
> +     <literal>frels_extra</literal> is same length with
> +     <literal>frels_list</literal>, that delivers extra information of
> +     the context where the foreign-tables are truncated.
> +    </para>
>
> Don't we need to document the detail information about frels_extra?
> Otherwise the developers of FDW would fail to understand how to
> handle the frels_extra when trying to make their FDWs support TRUNCATE.
>
>
> +               relids_extra = lappend_int(relids_extra, (recurse ? 0 : 1));
> +                               relids_extra = lappend_int(relids_extra, -1);
>
> postgres_fdw determines whether to specify ONLY or not by checking
> whether the passed extra value is zero or not. That is, for example,
> using only 0 and 1 for extra values is enough for the purpose. But
> ExecuteTruncate() sets three values 0, -1 and 1 as extra ones. Why are
> these three values necessary?
>
>
> With the patch, if both local and foreign tables are specified as
> the target tables to truncate, TRUNCATE command tries to truncate
> foreign tables after truncating local ones. That is, if "truncatable"
> option is set to false or enough permission to truncate is not granted
> yet in the foreign server, an error will be thrown after the local tables
> are truncated. I don't think this is good order of processings. IMO,
> instead, we should check whether foreign tables can be truncated
> before any actual truncation operations. For example, we can easily
> do that by truncate foreign tables before local ones. Thought?
>
>
> XLOG_HEAP_TRUNCATE record is written even for the truncation of
> a foreign table. Why is this necessary?
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/03/29 9:31, Kohei KaiGai wrote:
> Fujii-san,
> 
>> XLOG_HEAP_TRUNCATE record is written even for the truncation of
>> a foreign table. Why is this necessary?
>>
> Foreign-tables are often used to access local data structure, like
> columnar data files
> on filesystem, not only remote accesses like postgres_fdw.
> In case when we want to implement logical replication on this kind of
> foreign-tables,
> truncate-command must be delivered to subscriber node - to truncate
> its local data.
> 
> In case of remote-access FDW drivers, truncate-command on the subscriber-side is
> probably waste of cycles, however, only FDW driver and DBA who configured the
> foreign-table know whether it is necessary, or not.
> 
> How about your opinions?

I understand the motivation of this. But the other DMLs like UPDATE also
do the same thing for foreign tables? That is, when those DML commands
are executed on foreign tables, their changes are WAL-logged in a publisher side,
e.g., for logical replication? If not, it seems strange to allow only TRUNCATE
on foreign tables to be WAL-logged in a publisher side...

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Mon, Mar 29, 2021 at 10:53:14AM +0900, Fujii Masao wrote:
> I understand the motivation of this. But the other DMLs like UPDATE also
> do the same thing for foreign tables? That is, when those DML commands
> are executed on foreign tables, their changes are WAL-logged in a publisher side,
> e.g., for logical replication? If not, it seems strange to allow only TRUNCATE
> on foreign tables to be WAL-logged in a publisher side...

Executing DMLs on foreign tables does not generate any WAL AFAIK with
the backend core code, even with wal_level = logical, as the DML is
executed within the FDW callback (see just ExecUpdate() or
ExecInsert() in nodeModifyTable.c), and foreign tables don't have an
AM set as they have no physical storage.  A FDW may decide to generate
some WAL records by itself though when doing the opeation, using the
generic WAL interface but that's rather limited.

Generating WAL for the truncation of foreign tables sounds also like a
strange concept to me.  I think that you should just make the patch
work so as the truncation is passed down to the FDW that decides what
it needs to do with it, and do nothing more than that.
--
Michael

On 2021/03/28 2:37, Kazutaka Onishi wrote:
> Fujii-san,
> 
> Thank you for your review!
> Now I prepare v5 patch and I'll answer to your each comment. please
> check this again.

Thanks a lot!

> 5. For example, we can easily do that by truncate foreign tables
> before local ones. Thought?
> 
> Umm... yeah, I feel it's better procedure, but not so required because
> TRUNCATE is NOT called frequently.
> Certainly, we already have postgresIsForeignUpdatable() to check
> whether the foreign table is updatable or not.
> Following this way, we have to add postgresIsForeignTruncatable() to check.
> However, Unlike UPDATE, TRUNCATE is NOT called frequently. Current
> procedure is inefficient but works correctly.
> Thus, I feel postgresIsForeignTruncatable() is not needed.

I'm concerned about the case where permission errors at the remote servers
rather than that truncatable option is disabled. The comments of
ExecuteTruncate() explains its design as follows. But the patch seems to break
this because it truncates the local tables before checking the permission on
foreign tables (i.e., the local tables in remote servers)... No?

     We first open and grab exclusive
     lock on all relations involved, checking permissions and otherwise
     verifying that the relation is OK for truncation
     Finally all the relations are truncated and reindexed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/03/29 13:55, Michael Paquier wrote:
> On Mon, Mar 29, 2021 at 10:53:14AM +0900, Fujii Masao wrote:
>> I understand the motivation of this. But the other DMLs like UPDATE also
>> do the same thing for foreign tables? That is, when those DML commands
>> are executed on foreign tables, their changes are WAL-logged in a publisher side,
>> e.g., for logical replication? If not, it seems strange to allow only TRUNCATE
>> on foreign tables to be WAL-logged in a publisher side...
> 
> Executing DMLs on foreign tables does not generate any WAL AFAIK with
> the backend core code, even with wal_level = logical, as the DML is
> executed within the FDW callback (see just ExecUpdate() or
> ExecInsert() in nodeModifyTable.c), and foreign tables don't have an
> AM set as they have no physical storage.  A FDW may decide to generate
> some WAL records by itself though when doing the opeation, using the
> generic WAL interface but that's rather limited.
> 
> Generating WAL for the truncation of foreign tables sounds also like a
> strange concept to me.  I think that you should just make the patch
> work so as the truncation is passed down to the FDW that decides what
> it needs to do with it, and do nothing more than that.

Agreed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/03/28 2:37, Kazutaka Onishi wrote:
> Fujii-san,
> 
> Thank you for your review!
> Now I prepare v5 patch and I'll answer to your each comment. please
> check this again.
> m(_ _)m
> 
> 1. In postgres-fdw.sgml, "and truncatable" should be appended into the
> above first description?
> 2. truncate.sgml should be updated because, for example, it contains
> the above descriptions.
> 
> Yeah, you're right. I've fixed it.
> 
> 
> 
> 3.  Don't we need to document the detail information about frels_extra?
> 
> I've written about frels_extra into fdwhander.sgml.
> 
> 
> 
> 4. postgres_fdw determines whether to specify ONLY or not by checking
> whether the passed extra value is zero or not.
> 
> Please refer this:
> https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
>> Negative value means that foreign-tables are not specified in the TRUNCATE
>> command, but truncated due to dependency (like partition's child leaf).
> 
> I've added this information into fdwhandler.sgml.

Even when a foreign table is specified explicitly in TRUNCATE command,
its extra value can be negative if it's found as an inherited children firstly
(i.e., in the case where the partitioned table having that foreign table as
its partition is specified explicitly in TRUNCATE command).
Isn't this a problem?

Please imagine the following example;

----------------------------------
create extension postgres_fdw;
create server loopback foreign data wrapper postgres_fdw;
create user mapping for public server loopback;

create table t (i int, j int) partition by hash (j);
create table t0 partition of t for values with (modulus 2, remainder 0);
create table t1 partition of t for values with (modulus 2, remainder 1);

create table test (i int, j int) partition by hash (i);
create table test0 partition of test for values with (modulus 2, remainder 0);
create foreign table ft partition of test for values with (modulus 2, remainder 1) server loopback options (table_name
't');
----------------------------------

In this example, "truncate ft, test" works fine, but "truncate test, ft" causes
an error though they should work in the same way basically.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年3月30日(火) 2:54 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/03/29 13:55, Michael Paquier wrote:
> > On Mon, Mar 29, 2021 at 10:53:14AM +0900, Fujii Masao wrote:
> >> I understand the motivation of this. But the other DMLs like UPDATE also
> >> do the same thing for foreign tables? That is, when those DML commands
> >> are executed on foreign tables, their changes are WAL-logged in a publisher side,
> >> e.g., for logical replication? If not, it seems strange to allow only TRUNCATE
> >> on foreign tables to be WAL-logged in a publisher side...
> >
> > Executing DMLs on foreign tables does not generate any WAL AFAIK with
> > the backend core code, even with wal_level = logical, as the DML is
> > executed within the FDW callback (see just ExecUpdate() or
> > ExecInsert() in nodeModifyTable.c), and foreign tables don't have an
> > AM set as they have no physical storage.  A FDW may decide to generate
> > some WAL records by itself though when doing the opeation, using the
> > generic WAL interface but that's rather limited.
> >
> > Generating WAL for the truncation of foreign tables sounds also like a
> > strange concept to me.  I think that you should just make the patch
> > work so as the truncation is passed down to the FDW that decides what
> > it needs to do with it, and do nothing more than that.
>
> Agreed.
>
Ok, it's fair enough.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

2021年3月30日(火) 3:45 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/03/28 2:37, Kazutaka Onishi wrote:
> > Fujii-san,
> >
> > Thank you for your review!
> > Now I prepare v5 patch and I'll answer to your each comment. please
> > check this again.
> > m(_ _)m
> >
> > 1. In postgres-fdw.sgml, "and truncatable" should be appended into the
> > above first description?
> > 2. truncate.sgml should be updated because, for example, it contains
> > the above descriptions.
> >
> > Yeah, you're right. I've fixed it.
> >
> >
> >
> > 3.  Don't we need to document the detail information about frels_extra?
> >
> > I've written about frels_extra into fdwhander.sgml.
> >
> >
> >
> > 4. postgres_fdw determines whether to specify ONLY or not by checking
> > whether the passed extra value is zero or not.
> >
> > Please refer this:
> > https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
> >> Negative value means that foreign-tables are not specified in the TRUNCATE
> >> command, but truncated due to dependency (like partition's child leaf).
> >
> > I've added this information into fdwhandler.sgml.
>
> Even when a foreign table is specified explicitly in TRUNCATE command,
> its extra value can be negative if it's found as an inherited children firstly
> (i.e., in the case where the partitioned table having that foreign table as
> its partition is specified explicitly in TRUNCATE command).
> Isn't this a problem?
>
> Please imagine the following example;
>
> ----------------------------------
> create extension postgres_fdw;
> create server loopback foreign data wrapper postgres_fdw;
> create user mapping for public server loopback;
>
> create table t (i int, j int) partition by hash (j);
> create table t0 partition of t for values with (modulus 2, remainder 0);
> create table t1 partition of t for values with (modulus 2, remainder 1);
>
> create table test (i int, j int) partition by hash (i);
> create table test0 partition of test for values with (modulus 2, remainder 0);
> create foreign table ft partition of test for values with (modulus 2, remainder 1) server loopback options
(table_name't'); 
> ----------------------------------
>
> In this example, "truncate ft, test" works fine, but "truncate test, ft" causes
> an error though they should work in the same way basically.
>
(Although it was originally designed by me...)
If frels_extra would be a bit-masked value, we can avoid the problem.

Please assume the three labels below:
#define TRUNCATE_REL_CONTEXT__NORMAL         0x01
#define TRUNCATE_REL_CONTEXT__ONLY               0x02
#define TRUNCATE_REL_CONTEXT__CASCADED     0x04

Then, assign these labels on the extra flag according to the context where
the foreign-tables appeared in the truncate command.
Even if it is specified multiple times in the different context, FDW extension
can handle the best option according to the flags.

> In this example, "truncate ft, test" works fine, but "truncate test, ft" causes

In both cases, ExecForeignTruncate shall be invoked to "ft" with
(NORMAL | CASCADED),
thus, postgres_fdw can determine the remote truncate command shall be
executed without "ONLY" clause.

How about the idea?

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/03/30 10:11, Kohei KaiGai wrote:
> 2021年3月30日(火) 3:45 Fujii Masao <masao.fujii@oss.nttdata.com>:
>>
>> On 2021/03/28 2:37, Kazutaka Onishi wrote:
>>> Fujii-san,
>>>
>>> Thank you for your review!
>>> Now I prepare v5 patch and I'll answer to your each comment. please
>>> check this again.
>>> m(_ _)m
>>>
>>> 1. In postgres-fdw.sgml, "and truncatable" should be appended into the
>>> above first description?
>>> 2. truncate.sgml should be updated because, for example, it contains
>>> the above descriptions.
>>>
>>> Yeah, you're right. I've fixed it.
>>>
>>>
>>>
>>> 3.  Don't we need to document the detail information about frels_extra?
>>>
>>> I've written about frels_extra into fdwhander.sgml.
>>>
>>>
>>>
>>> 4. postgres_fdw determines whether to specify ONLY or not by checking
>>> whether the passed extra value is zero or not.
>>>
>>> Please refer this:
>>> https://www.postgresql.org/message-id/CAOP8fzb-t3WVNLjGMC%2B4sV4AZa9S%3DMAQ7Q6pQoADMCf_1jp4ew%40mail.gmail.com
>>>> Negative value means that foreign-tables are not specified in the TRUNCATE
>>>> command, but truncated due to dependency (like partition's child leaf).
>>>
>>> I've added this information into fdwhandler.sgml.
>>
>> Even when a foreign table is specified explicitly in TRUNCATE command,
>> its extra value can be negative if it's found as an inherited children firstly
>> (i.e., in the case where the partitioned table having that foreign table as
>> its partition is specified explicitly in TRUNCATE command).
>> Isn't this a problem?
>>
>> Please imagine the following example;
>>
>> ----------------------------------
>> create extension postgres_fdw;
>> create server loopback foreign data wrapper postgres_fdw;
>> create user mapping for public server loopback;
>>
>> create table t (i int, j int) partition by hash (j);
>> create table t0 partition of t for values with (modulus 2, remainder 0);
>> create table t1 partition of t for values with (modulus 2, remainder 1);
>>
>> create table test (i int, j int) partition by hash (i);
>> create table test0 partition of test for values with (modulus 2, remainder 0);
>> create foreign table ft partition of test for values with (modulus 2, remainder 1) server loopback options
(table_name't');
 
>> ----------------------------------
>>
>> In this example, "truncate ft, test" works fine, but "truncate test, ft" causes
>> an error though they should work in the same way basically.
>>
> (Although it was originally designed by me...)
> If frels_extra would be a bit-masked value, we can avoid the problem.
> 
> Please assume the three labels below:
> #define TRUNCATE_REL_CONTEXT__NORMAL         0x01
> #define TRUNCATE_REL_CONTEXT__ONLY               0x02
> #define TRUNCATE_REL_CONTEXT__CASCADED     0x04
> 
> Then, assign these labels on the extra flag according to the context where
> the foreign-tables appeared in the truncate command.
> Even if it is specified multiple times in the different context, FDW extension
> can handle the best option according to the flags.
> 
>> In this example, "truncate ft, test" works fine, but "truncate test, ft" causes
> 
> In both cases, ExecForeignTruncate shall be invoked to "ft" with
> (NORMAL | CASCADED),
> thus, postgres_fdw can determine the remote truncate command shall be
> executed without "ONLY" clause.
> 
> How about the idea?

This idea looks better to me.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年3月30日(火) 2:53 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/03/28 2:37, Kazutaka Onishi wrote:
> > Fujii-san,
> >
> > Thank you for your review!
> > Now I prepare v5 patch and I'll answer to your each comment. please
> > check this again.
>
> Thanks a lot!
>
> > 5. For example, we can easily do that by truncate foreign tables
> > before local ones. Thought?
> >
> > Umm... yeah, I feel it's better procedure, but not so required because
> > TRUNCATE is NOT called frequently.
> > Certainly, we already have postgresIsForeignUpdatable() to check
> > whether the foreign table is updatable or not.
> > Following this way, we have to add postgresIsForeignTruncatable() to check.
> > However, Unlike UPDATE, TRUNCATE is NOT called frequently. Current
> > procedure is inefficient but works correctly.
> > Thus, I feel postgresIsForeignTruncatable() is not needed.
>
> I'm concerned about the case where permission errors at the remote servers
> rather than that truncatable option is disabled. The comments of
> ExecuteTruncate() explains its design as follows. But the patch seems to break
> this because it truncates the local tables before checking the permission on
> foreign tables (i.e., the local tables in remote servers)... No?
>
>      We first open and grab exclusive
>      lock on all relations involved, checking permissions and otherwise
>      verifying that the relation is OK for truncation
>      Finally all the relations are truncated and reindexed.
>
Fujii-san,

What does the "permission checks" mean in this context?
The permission checks on the foreign tables involved are already checked
at truncate_check_rel(), by PostgreSQL's standard access control.

Please assume an extreme example below:
1. I defined a foreign table with file_fdw onto a local CSV file.
2. Someone tries to scan the foreign table, and ACL allows it.
3. I disallow the read remission of the CSV using chmod, after the above step,
    but prior to the query execution.
4. Someone's query moved to the execution stage, then IterateForeignScan()
    raises an error due to OS level permission checks.

FDW is a mechanism to convert from/to external data sources to/from PostgreSQL's
structured data, as literal. Once we checked the permissions of
foreign-tables by
database ACLs, any other permission checks handled by FDW driver are a part of
execution (like, OS permission check when file_fdw read(2) the
underlying CSV files).
And, we have no reliable way to check entire permissions preliminary,
like OS file
permission check or database permission checks by remote server. Even
if a checker
routine returned a "hint", it may be changed at the execution time.
Somebody might
change the "truncate" permission at the remote server.

How about your opinions?

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/01 0:09, Kohei KaiGai wrote:
> What does the "permission checks" mean in this context?
> The permission checks on the foreign tables involved are already checked
> at truncate_check_rel(), by PostgreSQL's standard access control.

I meant that's the permission check that happens in the remote server side.
For example, when the foreign table is defined on postgres_fdw and truncated,
TRUNCATE command is issued to the remote server via postgres_fdw and
it checks the permission of the table before performing actual truncation.


> Please assume an extreme example below:
> 1. I defined a foreign table with file_fdw onto a local CSV file.
> 2. Someone tries to scan the foreign table, and ACL allows it.
> 3. I disallow the read remission of the CSV using chmod, after the above step,
>      but prior to the query execution.
> 4. Someone's query moved to the execution stage, then IterateForeignScan()
>      raises an error due to OS level permission checks.
> 
> FDW is a mechanism to convert from/to external data sources to/from PostgreSQL's
> structured data, as literal. Once we checked the permissions of
> foreign-tables by
> database ACLs, any other permission checks handled by FDW driver are a part of
> execution (like, OS permission check when file_fdw read(2) the
> underlying CSV files).
> And, we have no reliable way to check entire permissions preliminary,
> like OS file
> permission check or database permission checks by remote server. Even
> if a checker
> routine returned a "hint", it may be changed at the execution time.
> Somebody might
> change the "truncate" permission at the remote server.
> 
> How about your opinions?

I agree that something like checker routine might not be so useful and
also be overkill. I was thinking that it's better to truncate the foreign tables first
and the local ones later. Otherwise it's a waste of cycles to truncate
the local tables if the truncation on foreign table causes an permission error
on the remote server.

But ISTM that the order of tables to truncate that the current patch provides
doesn't cause any actual bugs. So if you think the current order is better,
I'm ok with that for now. In this case, the comments for ExecuteTruncate()
should be updated.

BTW, the latest patch doesn't seem to be applied cleanly to the master
because of commit 27e1f14563. Could you rebase it?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月1日(木) 18:53 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/01 0:09, Kohei KaiGai wrote:
> > What does the "permission checks" mean in this context?
> > The permission checks on the foreign tables involved are already checked
> > at truncate_check_rel(), by PostgreSQL's standard access control.
>
> I meant that's the permission check that happens in the remote server side.
> For example, when the foreign table is defined on postgres_fdw and truncated,
> TRUNCATE command is issued to the remote server via postgres_fdw and
> it checks the permission of the table before performing actual truncation.
>
>
> > Please assume an extreme example below:
> > 1. I defined a foreign table with file_fdw onto a local CSV file.
> > 2. Someone tries to scan the foreign table, and ACL allows it.
> > 3. I disallow the read remission of the CSV using chmod, after the above step,
> >      but prior to the query execution.
> > 4. Someone's query moved to the execution stage, then IterateForeignScan()
> >      raises an error due to OS level permission checks.
> >
> > FDW is a mechanism to convert from/to external data sources to/from PostgreSQL's
> > structured data, as literal. Once we checked the permissions of
> > foreign-tables by
> > database ACLs, any other permission checks handled by FDW driver are a part of
> > execution (like, OS permission check when file_fdw read(2) the
> > underlying CSV files).
> > And, we have no reliable way to check entire permissions preliminary,
> > like OS file
> > permission check or database permission checks by remote server. Even
> > if a checker
> > routine returned a "hint", it may be changed at the execution time.
> > Somebody might
> > change the "truncate" permission at the remote server.
> >
> > How about your opinions?
>
> I agree that something like checker routine might not be so useful and
> also be overkill. I was thinking that it's better to truncate the foreign tables first
> and the local ones later. Otherwise it's a waste of cycles to truncate
> the local tables if the truncation on foreign table causes an permission error
> on the remote server.
>
> But ISTM that the order of tables to truncate that the current patch provides
> doesn't cause any actual bugs. So if you think the current order is better,
> I'm ok with that for now. In this case, the comments for ExecuteTruncate()
> should be updated.
>
It is fair enough for me to reverse the order of actual truncation.

How about the updated comments below?

    This is a multi-relation truncate.  We first open and grab exclusive
    lock on all relations involved, checking permissions (local database
    ACLs even if relations are foreign-tables) and otherwise verifying
    that the relation is OK for truncation. In CASCADE mode, ...(snip)...
    Finally all the relations are truncated and reindexed. If any foreign-
    tables are involved, its callback shall be invoked prior to the truncation
    of regular tables.

> BTW, the latest patch doesn't seem to be applied cleanly to the master
> because of commit 27e1f14563. Could you rebase it?
>
Onishi-san, go ahead. :-)

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/02 9:37, Kohei KaiGai wrote:
> It is fair enough for me to reverse the order of actual truncation.
> 
> How about the updated comments below?
> 
>      This is a multi-relation truncate.  We first open and grab exclusive
>      lock on all relations involved, checking permissions (local database
>      ACLs even if relations are foreign-tables) and otherwise verifying
>      that the relation is OK for truncation. In CASCADE mode, ...(snip)...
>      Finally all the relations are truncated and reindexed. If any foreign-
>      tables are involved, its callback shall be invoked prior to the truncation
>      of regular tables.

LGTM.


>> BTW, the latest patch doesn't seem to be applied cleanly to the master
>> because of commit 27e1f14563. Could you rebase it?
>>
> Onishi-san, go ahead. :-)

+1

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

All,

Thank you for discussion.
I've updated the patch (v6->v7) according to the conclusion.

I'll show the modified points:
1. Comments for ExecuteTuncate()
2. Replacing extra value in frels_extra with integer to label.
3. Skipping XLOG_HEAP_TRUNCATE on foreign table

Regards,

2021年4月2日(金) 11:44 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
>
>
> On 2021/04/02 9:37, Kohei KaiGai wrote:
> > It is fair enough for me to reverse the order of actual truncation.
> >
> > How about the updated comments below?
> >
> >      This is a multi-relation truncate.  We first open and grab exclusive
> >      lock on all relations involved, checking permissions (local database
> >      ACLs even if relations are foreign-tables) and otherwise verifying
> >      that the relation is OK for truncation. In CASCADE mode, ...(snip)...
> >      Finally all the relations are truncated and reindexed. If any foreign-
> >      tables are involved, its callback shall be invoked prior to the truncation
> >      of regular tables.
>
> LGTM.
>
>
> >> BTW, the latest patch doesn't seem to be applied cleanly to the master
> >> because of commit 27e1f14563. Could you rebase it?
> >>
> > Onishi-san, go ahead. :-)
>
> +1
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

Sorry but I found the v7 patch has typo and it can't be built...
I attached fixed one(v8).

2021年4月3日(土) 9:53 Kazutaka Onishi <onishi@heterodb.com>:
>
> All,
>
> Thank you for discussion.
> I've updated the patch (v6->v7) according to the conclusion.
>
> I'll show the modified points:
> 1. Comments for ExecuteTuncate()
> 2. Replacing extra value in frels_extra with integer to label.
> 3. Skipping XLOG_HEAP_TRUNCATE on foreign table
>
> Regards,
>
> 2021年4月2日(金) 11:44 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >
> >
> >
> > On 2021/04/02 9:37, Kohei KaiGai wrote:
> > > It is fair enough for me to reverse the order of actual truncation.
> > >
> > > How about the updated comments below?
> > >
> > >      This is a multi-relation truncate.  We first open and grab exclusive
> > >      lock on all relations involved, checking permissions (local database
> > >      ACLs even if relations are foreign-tables) and otherwise verifying
> > >      that the relation is OK for truncation. In CASCADE mode, ...(snip)...
> > >      Finally all the relations are truncated and reindexed. If any foreign-
> > >      tables are involved, its callback shall be invoked prior to the truncation
> > >      of regular tables.
> >
> > LGTM.
> >
> >
> > >> BTW, the latest patch doesn't seem to be applied cleanly to the master
> > >> because of commit 27e1f14563. Could you rebase it?
> > >>
> > > Onishi-san, go ahead. :-)
> >
> > +1
> >
> > Regards,
> >
> > --
> > Fujii Masao
> > Advanced Computing Technology Center
> > Research and Development Headquarters
> > NTT DATA CORPORATION

On Sat, Apr 3, 2021 at 7:16 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> Sorry but I found the v7 patch has typo and it can't be built...
> I attached fixed one(v8).

Thanks for the patch. Here are some comments on v8 patch:
1) We usually have the struct name after "+typedef struct
ForeignTruncateInfo", please refer to other struct defs in the code
base.

2) We should add ORDER BY clause(probably ORDER BY id?) for data
generating select queries in added tests, otherwise tests might become
unstable.

3) How about dropping the tables, foreign tables that got created for
testing in postgres_fdw.sql?

4) I think it's not "foreign-tables"/"foreign-table", it can be
"foreign tables"/"foreign table", other places in the docs use this
convention.
+     the context where the foreign-tables are truncated. It is a list
of integers and same length with

5) Can't we use do_sql_command function after making it non static? We
could go extra mile, that is we could make do_sql_command little more
generic by passing some enum for each of PQsendQuery,
PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
the respective code chunks with do_sql_command in postgres_fdw.c.

+    /* run remote query */
+    if (!PQsendQuery(conn, sql.data))
+        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
+    res = pgfdw_get_result(conn, sql.data);
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pgfdw_report_error(ERROR, res, conn, true, sql.data);
+    /* clean-up */
+    PQclear(res);

6) A white space error when the patch is applied.
contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.
+

7) I may be missing something here. Why do we need a hash table at
all? We could just do it with a linked list right? Is there a specific
reason to use a hash table? IIUC, the hash table entries will be lying
around until the local session exists since we are not doing
hash_destroy.

8) How about having something like this?
+    <command>TRUNCATE</command> can be used for foreign tables if the
foreign data wrapper supports, for instance, see <xref
linkend="postgres-fdw"/>.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:

Generally, sequential search would be slower if there are many entries
in a list. Here, the use case is to store all the foreign table ids
associated with each foreign server and I'm not sure how many foreign
tables will be provided in a single truncate command that belong to
different foreign servers. I strongly feel the count will be less and
using a list would be easier than to have a hash table. Others may
have better opinions.

> Should the hash table be released at the end of ExecuteTruncateGuts() ?

If we go with a hash table and think that the frequency of "TRUNCATE"
commands on foreign tables is heavy in a local session, then it does
make sense to not destroy the hash, otherwise destroy the hash.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

2021年4月4日(日) 13:07 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:
>
> Generally, sequential search would be slower if there are many entries
> in a list. Here, the use case is to store all the foreign table ids
> associated with each foreign server and I'm not sure how many foreign
> tables will be provided in a single truncate command that belong to
> different foreign servers. I strongly feel the count will be less and
> using a list would be easier than to have a hash table. Others may
> have better opinions.
>
https://www.postgresql.org/message-id/20200115081126.GK2243@paquier.xyz

It was originally implemented using a simple list, then modified according to
the comment by Michael.
I think it is just a matter of preference.

> > Should the hash table be released at the end of ExecuteTruncateGuts() ?
>
> If we go with a hash table and think that the frequency of "TRUNCATE"
> commands on foreign tables is heavy in a local session, then it does
> make sense to not destroy the hash, otherwise destroy the hash.
>
In most cases, TRUNCATE is not a command frequently executed.
So, exactly, it is just a matter of preference.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

Hi

For now, I've fixed the v8 according to your comments, excluding
anything related with 'hash table' and 'do_sql_commands'.

> 1) We usually have the struct name after "+typedef struct
> ForeignTruncateInfo", please refer to other struct defs in the code
> base.

I've modified the definition.
By the way, there're many "typedef struct{ ... }NameOfStruct;" in
codes, about 40% of other struct defs (checked by find&grep),
thus I felt the way is not "MUST".

> 2) We should add ORDER BY clause(probably ORDER BY id?) for data
> generating select queries in added tests, otherwise tests might become
> unstable.

I've added "ORDER BY" at the postges_fdw test.

> 3) How about dropping the tables, foreign tables that got created for
> testing in postgres_fdw.sql?

I've added "cleanup" commands.

> 4) I think it's not "foreign-tables"/"foreign-table", it can be
> "foreign tables"/"foreign table", other places in the docs use this
> convention.
> +     the context where the foreign-tables are truncated. It is a list
> of integers and same length with

I've replaced "foreign-table" to "foreign table".

> 5) Can't we use do_sql_command function after making it non static? We
> could go extra mile, that is we could make do_sql_command little more
> generic by passing some enum for each of PQsendQuery,
> PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
> the respective code chunks with do_sql_command in postgres_fdw.c.

I've skipped this for now.
I feel it sounds cool, but not easy.
It should be added by another patch because it's not only related to TRUNCATE.

> 6) A white space error when the patch is applied.
> contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.

I've checked the patch and clean spaces.
But I can't confirmed this message by attaching(patch -p1 < ...) my v8 patch.
If this still occurs, please tell me how you attach the patch.

> 7) I may be missing something here. Why do we need a hash table at
> all? We could just do it with a linked list right? Is there a specific
> reason to use a hash table? IIUC, the hash table entries will be lying
> around until the local session exists since we are not doing
> hash_destroy.

I've skipped this for now.


> 8) How about having something like this?
> +    <command>TRUNCATE</command> can be used for foreign tables if the
> foreign data wrapper supports, for instance, see <xref
> linkend="postgres-fdw"/>.

Sounds good. I've added.


9)
> +     <command>TRUNCATE</command> for each foreign server being involved
>
> +     in one <command>TRUNCATE</command> command (note that invocations
> The 'being' in above sentence can be omitted.

 I've fixed this.


10)
> +     the context where the foreign-tables are truncated. It is a list of integers and same length with
> There should be a verb between 'and' and same :
> It is a list of integers and has same length with

I've fixed this.

11)
> + * Information related to truncation of foreign tables.  This is used for
> + * the elements in a hash table that uses the server OID as lookup key,
> The 'uses' is for 'This' (the struct), so 'that' should be 'and':
> the elements in a hash table and uses
> Alternatively:
> the elements in a hash table. It uses

I've fixed this.

12)
> +       relids_extra = lappend_int(relids_extra, (recurse ? TRUNCATE_REL_CONTEXT__NORMAL :
TRUNCATE_REL_CONTEXT__ONLY));
> I am curious: isn't one underscore enough in the identifier (before NORMAL and ONLY) ?
> I suggest naming them TRUNCATE_REL_CONTEXT_NORMAL and TRUNCATE_REL_CONTEXT_ONLY

> +               relids_extra = lappend_int(relids_extra, TRUNCATE_REL_CONTEXT__CASCADED);
> I wonder if TRUNCATE_REL_CONTEXT_CASCADING is better than TRUNCATE_REL_CONTEXT__CASCADED. Note the removal of the
extraunderscore. 
> In English, we say: truncation cascading to foreign table.
> w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:

I've changed these labels shown below:
TRUNCATE_REL_CONTEXT__NORMAL -> TRUNCATE_REL_CONTEXT_NORMAL
TRUNCATE_REL_CONTEXT__ONLY -> TRUNCATE_REL_CONTEXT_ONLY
TRUNCATE_REL_CONTEXT__CASCADED -> TRUNCATE_REL_CONTEXT_CASCADING

14)
> +           ft_info = hash_search(ft_htab, &server_oid, HASH_ENTER, &found);
> and
> +       while ((ft_info = hash_seq_search(&seq)) != NULL)
> +    * Now go through the hash table, and process each entry associated to the
> +    * servers involved in the TRUNCATE.
> associated to -> associated with

I've fixed this.

14) Should the hash table be released at the end of ExecuteTruncateGuts() ?

I've skipped this for now.

2021年4月4日(日) 14:13 Kohei KaiGai <kaigai@heterodb.com>:
>
> 2021年4月4日(日) 13:07 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
> >
> > On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > > w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:
> >
> > Generally, sequential search would be slower if there are many entries
> > in a list. Here, the use case is to store all the foreign table ids
> > associated with each foreign server and I'm not sure how many foreign
> > tables will be provided in a single truncate command that belong to
> > different foreign servers. I strongly feel the count will be less and
> > using a list would be easier than to have a hash table. Others may
> > have better opinions.
> >
> https://www.postgresql.org/message-id/20200115081126.GK2243@paquier.xyz
>
> It was originally implemented using a simple list, then modified according to
> the comment by Michael.
> I think it is just a matter of preference.
>
> > > Should the hash table be released at the end of ExecuteTruncateGuts() ?
> >
> > If we go with a hash table and think that the frequency of "TRUNCATE"
> > commands on foreign tables is heavy in a local session, then it does
> > make sense to not destroy the hash, otherwise destroy the hash.
> >
> In most cases, TRUNCATE is not a command frequently executed.
> So, exactly, it is just a matter of preference.
>
> Best regards,
> --
> HeteroDB, Inc / The PG-Strom Project
> KaiGai Kohei <kaigai@heterodb.com>

v9 has also typo because I haven't checked about doc... sorry.

2021年4月4日(日) 15:30 Kazutaka Onishi <onishi@heterodb.com>:
>
> Hi
>
> For now, I've fixed the v8 according to your comments, excluding
> anything related with 'hash table' and 'do_sql_commands'.
>
> > 1) We usually have the struct name after "+typedef struct
> > ForeignTruncateInfo", please refer to other struct defs in the code
> > base.
>
> I've modified the definition.
> By the way, there're many "typedef struct{ ... }NameOfStruct;" in
> codes, about 40% of other struct defs (checked by find&grep),
> thus I felt the way is not "MUST".
>
> > 2) We should add ORDER BY clause(probably ORDER BY id?) for data
> > generating select queries in added tests, otherwise tests might become
> > unstable.
>
> I've added "ORDER BY" at the postges_fdw test.
>
> > 3) How about dropping the tables, foreign tables that got created for
> > testing in postgres_fdw.sql?
>
> I've added "cleanup" commands.
>
> > 4) I think it's not "foreign-tables"/"foreign-table", it can be
> > "foreign tables"/"foreign table", other places in the docs use this
> > convention.
> > +     the context where the foreign-tables are truncated. It is a list
> > of integers and same length with
>
> I've replaced "foreign-table" to "foreign table".
>
> > 5) Can't we use do_sql_command function after making it non static? We
> > could go extra mile, that is we could make do_sql_command little more
> > generic by passing some enum for each of PQsendQuery,
> > PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
> > the respective code chunks with do_sql_command in postgres_fdw.c.
>
> I've skipped this for now.
> I feel it sounds cool, but not easy.
> It should be added by another patch because it's not only related to TRUNCATE.
>
> > 6) A white space error when the patch is applied.
> > contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.
>
> I've checked the patch and clean spaces.
> But I can't confirmed this message by attaching(patch -p1 < ...) my v8 patch.
> If this still occurs, please tell me how you attach the patch.
>
> > 7) I may be missing something here. Why do we need a hash table at
> > all? We could just do it with a linked list right? Is there a specific
> > reason to use a hash table? IIUC, the hash table entries will be lying
> > around until the local session exists since we are not doing
> > hash_destroy.
>
> I've skipped this for now.
>
>
> > 8) How about having something like this?
> > +    <command>TRUNCATE</command> can be used for foreign tables if the
> > foreign data wrapper supports, for instance, see <xref
> > linkend="postgres-fdw"/>.
>
> Sounds good. I've added.
>
>
> 9)
> > +     <command>TRUNCATE</command> for each foreign server being involved
> >
> > +     in one <command>TRUNCATE</command> command (note that invocations
> > The 'being' in above sentence can be omitted.
>
>  I've fixed this.
>
>
> 10)
> > +     the context where the foreign-tables are truncated. It is a list of integers and same length with
> > There should be a verb between 'and' and same :
> > It is a list of integers and has same length with
>
> I've fixed this.
>
> 11)
> > + * Information related to truncation of foreign tables.  This is used for
> > + * the elements in a hash table that uses the server OID as lookup key,
> > The 'uses' is for 'This' (the struct), so 'that' should be 'and':
> > the elements in a hash table and uses
> > Alternatively:
> > the elements in a hash table. It uses
>
> I've fixed this.
>
> 12)
> > +       relids_extra = lappend_int(relids_extra, (recurse ? TRUNCATE_REL_CONTEXT__NORMAL :
TRUNCATE_REL_CONTEXT__ONLY));
> > I am curious: isn't one underscore enough in the identifier (before NORMAL and ONLY) ?
> > I suggest naming them TRUNCATE_REL_CONTEXT_NORMAL and TRUNCATE_REL_CONTEXT_ONLY
>
> > +               relids_extra = lappend_int(relids_extra, TRUNCATE_REL_CONTEXT__CASCADED);
> > I wonder if TRUNCATE_REL_CONTEXT_CASCADING is better than TRUNCATE_REL_CONTEXT__CASCADED. Note the removal of the
extraunderscore. 
> > In English, we say: truncation cascading to foreign table.
> > w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:
>
> I've changed these labels shown below:
> TRUNCATE_REL_CONTEXT__NORMAL -> TRUNCATE_REL_CONTEXT_NORMAL
> TRUNCATE_REL_CONTEXT__ONLY -> TRUNCATE_REL_CONTEXT_ONLY
> TRUNCATE_REL_CONTEXT__CASCADED -> TRUNCATE_REL_CONTEXT_CASCADING
>
> 14)
> > +           ft_info = hash_search(ft_htab, &server_oid, HASH_ENTER, &found);
> > and
> > +       while ((ft_info = hash_seq_search(&seq)) != NULL)
> > +    * Now go through the hash table, and process each entry associated to the
> > +    * servers involved in the TRUNCATE.
> > associated to -> associated with
>
> I've fixed this.
>
> 14) Should the hash table be released at the end of ExecuteTruncateGuts() ?
>
> I've skipped this for now.
>
> 2021年4月4日(日) 14:13 Kohei KaiGai <kaigai@heterodb.com>:
> >
> > 2021年4月4日(日) 13:07 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
> > >
> > > On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu <zyu@yugabyte.com> wrote:
> > > > w.r.t. Bharath's question on using hash table, I think the reason is that the search would be more efficient:
> > >
> > > Generally, sequential search would be slower if there are many entries
> > > in a list. Here, the use case is to store all the foreign table ids
> > > associated with each foreign server and I'm not sure how many foreign
> > > tables will be provided in a single truncate command that belong to
> > > different foreign servers. I strongly feel the count will be less and
> > > using a list would be easier than to have a hash table. Others may
> > > have better opinions.
> > >
> > https://www.postgresql.org/message-id/20200115081126.GK2243@paquier.xyz
> >
> > It was originally implemented using a simple list, then modified according to
> > the comment by Michael.
> > I think it is just a matter of preference.
> >
> > > > Should the hash table be released at the end of ExecuteTruncateGuts() ?
> > >
> > > If we go with a hash table and think that the frequency of "TRUNCATE"
> > > commands on foreign tables is heavy in a local session, then it does
> > > make sense to not destroy the hash, otherwise destroy the hash.
> > >
> > In most cases, TRUNCATE is not a command frequently executed.
> > So, exactly, it is just a matter of preference.
> >
> > Best regards,
> > --
> > HeteroDB, Inc / The PG-Strom Project
> > KaiGai Kohei <kaigai@heterodb.com>

On Sun, Apr 4, 2021 at 12:48 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> v9 has also typo because I haven't checked about doc... sorry.

I think v9 has some changes not related to the foreign table truncate
feature. If yes, please remove those changes and provide a proper
patch.

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
diff --git a/src/backend/bootstrap/bootstrap.c
b/src/backend/bootstrap/bootstrap.c
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
....
....

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Oops... sorry.
I haven't merged my working git branch with remote master branch.
Please check this v11.

2021年4月4日(日) 23:56 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Sun, Apr 4, 2021 at 12:48 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> >
> > v9 has also typo because I haven't checked about doc... sorry.
>
> I think v9 has some changes not related to the foreign table truncate
> feature. If yes, please remove those changes and provide a proper
> patch.
>
> diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
> diff --git a/src/backend/bootstrap/bootstrap.c
> b/src/backend/bootstrap/bootstrap.c
> diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
> ....
> ....
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > 5) Can't we use do_sql_command function after making it non static? We
> > could go extra mile, that is we could make do_sql_command little more
> > generic by passing some enum for each of PQsendQuery,
> > PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
> > the respective code chunks with do_sql_command in postgres_fdw.c.
>
> I've skipped this for now.
> I feel it sounds cool, but not easy.
> It should be added by another patch because it's not only related to TRUNCATE.

Fair enough! I will give it a thought and provide a patch separately.

> > 6) A white space error when the patch is applied.
> > contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.
>
> I've checked the patch and clean spaces.
> But I can't confirmed this message by attaching(patch -p1 < ...) my v8 patch.
> If this still occurs, please tell me how you attach the patch.

I usually follow these steps:
1) write code 2) git diff --check (will give if there are any white
space or indentation errors) 3) git add -u 4) git commit (will enter a
commit message) 5) git format-patch -1 <<sha of the commit>> -v
<<version number>> 6) to apply patch, git apply <<patch_name>>.patch

> > 7) I may be missing something here. Why do we need a hash table at
> > all? We could just do it with a linked list right? Is there a specific
> > reason to use a hash table? IIUC, the hash table entries will be lying
> > around until the local session exists since we are not doing
> > hash_destroy.
>
> I've skipped this for now.

If you don't destroy the hash, you are going to cause a memory leak.
Because, the pointer to hash tableft_htab is local to
ExecuteTruncateGuts (note that it's not a static variable) and you are
creating a memory for the hash table and leaving the function without
cleaning it up. IMHO, we should destroy the hash memory at the end of
ExecuteTruncateGuts.

Another comment for tests, why do we need to do full outer join of two
tables to just show up there are some rows in the table? I would
suggest that all the tests introduced in the patch can be something
like this: 1) before truncate, just show the count(*) from the table
2) truncate the foreign tables 3) after truncate, just show the
count(*) which should be 0. Because we don't care what the data is in
the tables, we only care about whether truncate is happened or not.

+SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
b.id ORDER BY a.id;
+ id |                x                 | id |                z
+----+----------------------------------+----+----------------------------------
+  1 | eccbc87e4b5ce2fe28308fd9f2a7baf3 |    |
+  2 | a87ff679a2f3e71d9181a67b7542122c |    |
+  3 | e4da3b7fbbce2345d7772b0674a318d5 |  3 | 1679091c5a880faf6fb5e6087eb1b2dc
+  4 | 1679091c5a880faf6fb5e6087eb1b2dc |  4 | 8f14e45fceea167a5a36dedd4bea2543
+  5 | 8f14e45fceea167a5a36dedd4bea2543 |  5 | c9f0f895fb98ab9159f51fd0297e236d
+  6 | c9f0f895fb98ab9159f51fd0297e236d |  6 | 45c48cce2e2d7fbdea1afc51c7c6ad26
+  7 | 45c48cce2e2d7fbdea1afc51c7c6ad26 |  7 | d3d9446802a44259755d38e6d163e820
+  8 | d3d9446802a44259755d38e6d163e820 |  8 | 6512bd43d9caa6e02c990b0a82652dca
+    |                                  |  9 | c20ad4d76fe97759aa27a0c99bff6710
+    |                                  | 10 | c51ce410c124a10e0db5e4b97fc2af39
+(10 rows)
+
+TRUNCATE tru_ftable, tru_pk_ftable CASCADE;
+SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
b.id ORDER BY a.id;  -- empty


With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Thank you for checking v11!
I've updated it and attached v12.

> I usually follow these steps:
> 1) write code 2) git diff --check (will give if there are any white
> space or indentation errors) 3) git add -u 4) git commit (will enter a
> commit message) 5) git format-patch -1 <<sha of the commit>> -v
> <<version number>> 6) to apply patch, git apply <<patch_name>>.patch

thanks, I've removed these whitespaces and confirmed no warnings
occurred when I run "git apply <<patch_name>>.patch"

> If you don't destroy the hash, you are going to cause a memory leak.
> Because, the pointer to hash tableft_htab is local to
> ExecuteTruncateGuts (note that it's not a static variable) and you are
> creating a memory for the hash table and leaving the function without
> cleaning it up. IMHO, we should destroy the hash memory at the end of
> ExecuteTruncateGuts.

Sure. I've added head_destroy().

> Another comment for tests, why do we need to do full outer join of two
> tables to just show up there are some rows in the table? I would
> suggest that all the tests introduced in the patch can be something
> like this: 1) before truncate, just show the count(*) from the table
> 2) truncate the foreign tables 3) after truncate, just show the
> count(*) which should be 0. Because we don't care what the data is in
> the tables, we only care about whether truncate is happened or not.

Sure. I've replaced with the test command "SELECT * FROM ..." to
"SELECT COUNT(*) FROM ..."
However, for example, the "id" column is used to check after running
TRUNCATE with ONLY clause to the inherited table.
Thus, I use "sum(id)" instead of "count(*)"  to check the result when
the table has records.

2021年4月5日(月) 0:20 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > > 5) Can't we use do_sql_command function after making it non static? We
> > > could go extra mile, that is we could make do_sql_command little more
> > > generic by passing some enum for each of PQsendQuery,
> > > PQsendQueryParams, PQsendQueryPrepared and PQsendPrepare and replace
> > > the respective code chunks with do_sql_command in postgres_fdw.c.
> >
> > I've skipped this for now.
> > I feel it sounds cool, but not easy.
> > It should be added by another patch because it's not only related to TRUNCATE.
>
> Fair enough! I will give it a thought and provide a patch separately.
>
> > > 6) A white space error when the patch is applied.
> > > contrib/postgres_fdw/postgres_fdw.c:2913: trailing whitespace.
> >
> > I've checked the patch and clean spaces.
> > But I can't confirmed this message by attaching(patch -p1 < ...) my v8 patch.
> > If this still occurs, please tell me how you attach the patch.
>
> I usually follow these steps:
> 1) write code 2) git diff --check (will give if there are any white
> space or indentation errors) 3) git add -u 4) git commit (will enter a
> commit message) 5) git format-patch -1 <<sha of the commit>> -v
> <<version number>> 6) to apply patch, git apply <<patch_name>>.patch
>
> > > 7) I may be missing something here. Why do we need a hash table at
> > > all? We could just do it with a linked list right? Is there a specific
> > > reason to use a hash table? IIUC, the hash table entries will be lying
> > > around until the local session exists since we are not doing
> > > hash_destroy.
> >
> > I've skipped this for now.
>
> If you don't destroy the hash, you are going to cause a memory leak.
> Because, the pointer to hash tableft_htab is local to
> ExecuteTruncateGuts (note that it's not a static variable) and you are
> creating a memory for the hash table and leaving the function without
> cleaning it up. IMHO, we should destroy the hash memory at the end of
> ExecuteTruncateGuts.
>
> Another comment for tests, why do we need to do full outer join of two
> tables to just show up there are some rows in the table? I would
> suggest that all the tests introduced in the patch can be something
> like this: 1) before truncate, just show the count(*) from the table
> 2) truncate the foreign tables 3) after truncate, just show the
> count(*) which should be 0. Because we don't care what the data is in
> the tables, we only care about whether truncate is happened or not.
>
> +SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
> b.id ORDER BY a.id;
> + id |                x                 | id |                z
> +----+----------------------------------+----+----------------------------------
> +  1 | eccbc87e4b5ce2fe28308fd9f2a7baf3 |    |
> +  2 | a87ff679a2f3e71d9181a67b7542122c |    |
> +  3 | e4da3b7fbbce2345d7772b0674a318d5 |  3 | 1679091c5a880faf6fb5e6087eb1b2dc
> +  4 | 1679091c5a880faf6fb5e6087eb1b2dc |  4 | 8f14e45fceea167a5a36dedd4bea2543
> +  5 | 8f14e45fceea167a5a36dedd4bea2543 |  5 | c9f0f895fb98ab9159f51fd0297e236d
> +  6 | c9f0f895fb98ab9159f51fd0297e236d |  6 | 45c48cce2e2d7fbdea1afc51c7c6ad26
> +  7 | 45c48cce2e2d7fbdea1afc51c7c6ad26 |  7 | d3d9446802a44259755d38e6d163e820
> +  8 | d3d9446802a44259755d38e6d163e820 |  8 | 6512bd43d9caa6e02c990b0a82652dca
> +    |                                  |  9 | c20ad4d76fe97759aa27a0c99bff6710
> +    |                                  | 10 | c51ce410c124a10e0db5e4b97fc2af39
> +(10 rows)
> +
> +TRUNCATE tru_ftable, tru_pk_ftable CASCADE;
> +SELECT * FROM tru_ftable a FULL OUTER JOIN tru_pk_ftable b ON a.id =
> b.id ORDER BY a.id;  -- empty
>
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Sun, Apr 4, 2021 at 10:23 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> Sure. I've replaced with the test command "SELECT * FROM ..." to
> "SELECT COUNT(*) FROM ..."
> However, for example, the "id" column is used to check after running
> TRUNCATE with ONLY clause to the inherited table.
> Thus, I use "sum(id)" instead of "count(*)"  to check the result when
> the table has records.

I still don't understand why we need sum(id), not count(*). Am I
missing something here?

Here are some more comments on the v12 patch:
1) Instead of switch case, a simple if else clause would reduce the code a bit:
    if (behavior == DROP_RESTRICT)
        appendStringInfoString(buf, " RESTRICT");
    else if (behavior == DROP_CASCADE)
        appendStringInfoString(buf, " CASCADE");

2) Some coding style comments:
It's better to have a new line after variable declarations,
assignments, function calls, before if blocks, after if blocks for
better readability of the code.
+    appendStringInfoString(buf, "TRUNCATE ");   ---> new line after this
+    forboth (lc1, frels_list,

+    }         ---> new line after this
+    appendStringInfo(buf, " %s IDENTITY",

+        /* ensure the target foreign table is truncatable */
+        truncatable = server_truncatable;    ---> new line after this
+        foreach (cell, ft->options)

+        }    ---> new line after this
+        if (!truncatable)

+    }    ---> new line after this
+    /* set up remote query */
+    initStringInfo(&sql);
+    deparseTruncateSql(&sql, frels_list, frels_extra, behavior,
restart_seqs);    ---> new line after this
+    /* run remote query */
+    if (!PQsendQuery(conn, sql.data))
+        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
---> new line after this
+    res = pgfdw_get_result(conn, sql.data);    ---> new line after this
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pgfdw_report_error(ERROR, res, conn, true, sql.data);    --->
new line after this
+    /* clean-up */
+    PQclear(res);
+    pfree(sql.data);
+}

and so on.

a space after "false," and before "NULL"
+            conn = GetConnection(user, false,NULL);

bring lc2, frels_extra to the same of lc1, frels_list
+    forboth (lc1, frels_list,
+             lc2, frels_extra)

3) I think we need truncatable behaviour that is consistent with updatable.
With your patch, seems like below is the behaviour for truncatable:
both server and foreign table are truncatable = truncated
server is not truncatable and foreign table is truncatable = not
truncated and error out
server is truncatable and foreign table is not truncatable = not
truncated and error out
server is not truncatable and foreign table is not truncatable = not
truncated and error out

Below is the behaviour for updatable:
both server and foreign table are updatable = updated
server is not updatable and foreign table is updatable = updated
server is updatable and foreign table is not updatable = not updated
server is not updatable and foreign table is not updatable = not updated

And also see comment in postgresIsForeignRelUpdatable
    /*
     * By default, all postgres_fdw foreign tables are assumed updatable. This
     * can be overridden by a per-server setting, which in turn can be
     * overridden by a per-table setting.
     */

IMO, you could do the same thing for truncatable, change is required
in your patch:
both server and foreign table are truncatable = truncated
server is not truncatable and foreign table is truncatable = truncated
server is truncatable and foreign table is not truncatable = not
truncated and error out
server is not truncatable and foreign table is not truncatable = not
truncated and error out

4) GetConnection needs to be done after all the error checking code
otherwise on error we would have opened a new connection without
actually using it. Just move below code outside of the for loop in
postgresExecForeignTruncate
+            user = GetUserMapping(GetUserId(), server_id);
+            conn = GetConnection(user, false,NULL);
to here:
+    Assert (OidIsValid(server_id)));
+
+    /* get a connection to the server */
+    user = GetUserMapping(GetUserId(), server_id);
+    conn = GetConnection(user, false, NULL);
+
+    /* set up remote query */
+    initStringInfo(&sql);
+    deparseTruncateSql(&sql, frels_list, frels_extra, behavior, restart_seqs);
+    /* run remote query */
+    if (!PQsendQuery(conn, sql.data))
+        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
+    res = pgfdw_get_result(conn, sql.data);
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pgfdw_report_error(ERROR, res, conn, true, sql.data);

5) This assertion is bogus, because GetForeignServerIdByRelId will
return valid server id and otherwise it fails with "cache lookup
error", so please remove it.
+        else
+        {
+            /* postgresExecForeignTruncate() is invoked for each server */
+            Assert(server_id == GetForeignServerIdByRelId(frel_oid));
+        }

6) You can add a comment here saying this if-clause gets executed only
once per server.
+
+        if (!OidIsValid(server_id))
+        {
+            server_id = GetForeignServerIdByRelId(frel_oid);

7) Did you try checking whether we reach hash_destroy code when a
failure happens on executing truncate on a remote table? Otherwise we
might want to do routine->ExecForeignTruncate inside PG_TRY block?
+            /* truncate_check_rel() has checked that already */
+            Assert(routine->ExecForeignTruncate != NULL);
+
+            routine->ExecForeignTruncate(ft_info->frels_list,
+                                         ft_info->frels_extra,
+                                         behavior,
+                                         restart_seqs);
+        }
+
+        hash_destroy(ft_htab);
+    }

8) This comment can be removed and have more descriptive comment
before the for loop in postgresExecForeignTruncate similar to the
comment what we have in postgresIsForeignRelUpdatable for updatable.
+ /* pick up remote connection, and sanity checks */

9) It will be good if you can divide up your patch into 3 separate
patches - 0001 code, 0002 tests, 0003 documentation

10) Why do we need many new tables for tests? Can't we do this -
insert, show count(*) as non-zero, truncate, show count(*) as 0, again
insert, another truncate test? And we can also have a very minimal
number of rows say 1 or 2 not 10? If possible, reduce the number of
new tables introduced. And do you have a specific reason to have a
text column in each of the tables? AFAICS, we don't care about the
column type, you could just have another int column and use
generate_series while inserting. We can remove md5 function calls.
Your tests will look clean.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Thank you for your comments.
I've attached v13.

> Here are some more comments on the v12 patch:
> I still don't understand why we need sum(id), not count(*). Am I
> missing something here?

The value of "id" is used for checking whether correct records are
truncated or not.
For instance, on "truncate with ONLY clause",
At first, There are 16 values in "tru_ftable_parent", for instance,
[1,2,3,...,8,10,11,12,...,18].
By "TRUNCATE ONLY tru_ftable_parent", [1,2,3,...,8] will be truncated.
Thus, the "sum(id)" = 126.
If we use "count(*)" here, then the value will be 8.
Let's consider the case that there are 8 values [1,2,3,...,8] by some
kind of bug after running "TRUNCATE ONLY tru_ftable_parent".
Then, we miss this bug by "count(*)" because the value is the same as
the correct pattern.

> 1) Instead of switch case, a simple if else clause would reduce the code a bit:
>     if (behavior == DROP_RESTRICT)
>         appendStringInfoString(buf, " RESTRICT");
>     else if (behavior == DROP_CASCADE)
>         appendStringInfoString(buf, " CASCADE");

I've modified it.


> 2) Some coding style comments:
> It's better to have a new line after variable declarations,
> assignments, function calls, before if blocks, after if blocks for
> better readability of the code.

I've fixed it.

> 3) I think we need truncatable behaviour that is consistent with updatable.

It's not correct. "truncatable" option works the same as "updatable".
I've confirmed that the table can be truncated with the combination:
truncatable on the server setting is false & truncatable on the table
setting is true.
I've also added to the test.

> 4) GetConnection needs to be done after all the error checking code
> otherwise on error we would have opened a new connection without
> actually using it. Just move below code outside of the for loop in
> postgresExecForeignTruncate

Sure, I've moved it.


> 5) This assertion is bogus, because GetForeignServerIdByRelId will
> return valid server id and otherwise it fails with "cache lookup
> error", so please remove it.

I've removed it.

> 6) You can add a comment here saying this if-clause gets executed only
> once per server.

I've added it.


> 7) Did you try checking whether we reach hash_destroy code when a
> failure happens on executing truncate on a remote table? Otherwise we
> might want to do routine->ExecForeignTruncate inside PG_TRY block?

I've added PG_TRY block.


> 8) This comment can be removed and have more descriptive comment
> before the for loop in postgresExecForeignTruncate similar to the
> comment what we have in postgresIsForeignRelUpdatable for updatable.

I've removed the comment and copied the comment from
postgresIsForeignRelUpdatable,
and modified it.

> 9) It will be good if you can divide up your patch into 3 separate
> patches - 0001 code, 0002 tests, 0003 documentation

I'll do it when I send a patch in the future, please forgive me on this patch.

> 10) Why do we need many new tables for tests? Can't we do this -
> insert, show count(*) as non-zero, truncate, show count(*) as 0, again
> insert, another truncate test? And we can also have a very minimal
> number of rows say 1 or 2 not 10? If possible, reduce the number of
> new tables introduced. And do you have a specific reason to have a
> text column in each of the tables? AFAICS, we don't care about the
> column type, you could just have another int column and use
> generate_series while inserting. We can remove md5 function calls.
> Your tests will look clean.

I've removed the text field but the number of records are kept.
As I say at the top, the value of id is checked so I want to keep the
number of rows.

2021年4月5日(月) 14:59 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Sun, Apr 4, 2021 at 10:23 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > Sure. I've replaced with the test command "SELECT * FROM ..." to
> > "SELECT COUNT(*) FROM ..."
> > However, for example, the "id" column is used to check after running
> > TRUNCATE with ONLY clause to the inherited table.
> > Thus, I use "sum(id)" instead of "count(*)"  to check the result when
> > the table has records.
>
> I still don't understand why we need sum(id), not count(*). Am I
> missing something here?
>
> Here are some more comments on the v12 patch:
> 1) Instead of switch case, a simple if else clause would reduce the code a bit:
>     if (behavior == DROP_RESTRICT)
>         appendStringInfoString(buf, " RESTRICT");
>     else if (behavior == DROP_CASCADE)
>         appendStringInfoString(buf, " CASCADE");
>
> 2) Some coding style comments:
> It's better to have a new line after variable declarations,
> assignments, function calls, before if blocks, after if blocks for
> better readability of the code.
> +    appendStringInfoString(buf, "TRUNCATE ");   ---> new line after this
> +    forboth (lc1, frels_list,
>
> +    }         ---> new line after this
> +    appendStringInfo(buf, " %s IDENTITY",
>
> +        /* ensure the target foreign table is truncatable */
> +        truncatable = server_truncatable;    ---> new line after this
> +        foreach (cell, ft->options)
>
> +        }    ---> new line after this
> +        if (!truncatable)
>
> +    }    ---> new line after this
> +    /* set up remote query */
> +    initStringInfo(&sql);
> +    deparseTruncateSql(&sql, frels_list, frels_extra, behavior,
> restart_seqs);    ---> new line after this
> +    /* run remote query */
> +    if (!PQsendQuery(conn, sql.data))
> +        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
> ---> new line after this
> +    res = pgfdw_get_result(conn, sql.data);    ---> new line after this
> +    if (PQresultStatus(res) != PGRES_COMMAND_OK)
> +        pgfdw_report_error(ERROR, res, conn, true, sql.data);    --->
> new line after this
> +    /* clean-up */
> +    PQclear(res);
> +    pfree(sql.data);
> +}
>
> and so on.
>
> a space after "false," and before "NULL"
> +            conn = GetConnection(user, false,NULL);
>
> bring lc2, frels_extra to the same of lc1, frels_list
> +    forboth (lc1, frels_list,
> +             lc2, frels_extra)
>
> 3) I think we need truncatable behaviour that is consistent with updatable.
> With your patch, seems like below is the behaviour for truncatable:
> both server and foreign table are truncatable = truncated
> server is not truncatable and foreign table is truncatable = not
> truncated and error out
> server is truncatable and foreign table is not truncatable = not
> truncated and error out
> server is not truncatable and foreign table is not truncatable = not
> truncated and error out
>
> Below is the behaviour for updatable:
> both server and foreign table are updatable = updated
> server is not updatable and foreign table is updatable = updated
> server is updatable and foreign table is not updatable = not updated
> server is not updatable and foreign table is not updatable = not updated
>
> And also see comment in postgresIsForeignRelUpdatable
>     /*
>      * By default, all postgres_fdw foreign tables are assumed updatable. This
>      * can be overridden by a per-server setting, which in turn can be
>      * overridden by a per-table setting.
>      */
>
> IMO, you could do the same thing for truncatable, change is required
> in your patch:
> both server and foreign table are truncatable = truncated
> server is not truncatable and foreign table is truncatable = truncated
> server is truncatable and foreign table is not truncatable = not
> truncated and error out
> server is not truncatable and foreign table is not truncatable = not
> truncated and error out
>
> 4) GetConnection needs to be done after all the error checking code
> otherwise on error we would have opened a new connection without
> actually using it. Just move below code outside of the for loop in
> postgresExecForeignTruncate
> +            user = GetUserMapping(GetUserId(), server_id);
> +            conn = GetConnection(user, false,NULL);
> to here:
> +    Assert (OidIsValid(server_id)));
> +
> +    /* get a connection to the server */
> +    user = GetUserMapping(GetUserId(), server_id);
> +    conn = GetConnection(user, false, NULL);
> +
> +    /* set up remote query */
> +    initStringInfo(&sql);
> +    deparseTruncateSql(&sql, frels_list, frels_extra, behavior, restart_seqs);
> +    /* run remote query */
> +    if (!PQsendQuery(conn, sql.data))
> +        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
> +    res = pgfdw_get_result(conn, sql.data);
> +    if (PQresultStatus(res) != PGRES_COMMAND_OK)
> +        pgfdw_report_error(ERROR, res, conn, true, sql.data);
>
> 5) This assertion is bogus, because GetForeignServerIdByRelId will
> return valid server id and otherwise it fails with "cache lookup
> error", so please remove it.
> +        else
> +        {
> +            /* postgresExecForeignTruncate() is invoked for each server */
> +            Assert(server_id == GetForeignServerIdByRelId(frel_oid));
> +        }
>
> 6) You can add a comment here saying this if-clause gets executed only
> once per server.
> +
> +        if (!OidIsValid(server_id))
> +        {
> +            server_id = GetForeignServerIdByRelId(frel_oid);
>
> 7) Did you try checking whether we reach hash_destroy code when a
> failure happens on executing truncate on a remote table? Otherwise we
> might want to do routine->ExecForeignTruncate inside PG_TRY block?
> +            /* truncate_check_rel() has checked that already */
> +            Assert(routine->ExecForeignTruncate != NULL);
> +
> +            routine->ExecForeignTruncate(ft_info->frels_list,
> +                                         ft_info->frels_extra,
> +                                         behavior,
> +                                         restart_seqs);
> +        }
> +
> +        hash_destroy(ft_htab);
> +    }
>
> 8) This comment can be removed and have more descriptive comment
> before the for loop in postgresExecForeignTruncate similar to the
> comment what we have in postgresIsForeignRelUpdatable for updatable.
> + /* pick up remote connection, and sanity checks */
>
> 9) It will be good if you can divide up your patch into 3 separate
> patches - 0001 code, 0002 tests, 0003 documentation
>
> 10) Why do we need many new tables for tests? Can't we do this -
> insert, show count(*) as non-zero, truncate, show count(*) as 0, again
> insert, another truncate test? And we can also have a very minimal
> number of rows say 1 or 2 not 10? If possible, reduce the number of
> new tables introduced. And do you have a specific reason to have a
> text column in each of the tables? AFAICS, we don't care about the
> column type, you could just have another int column and use
> generate_series while inserting. We can remove md5 function calls.
> Your tests will look clean.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 5, 2021 at 7:38 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > 3) I think we need truncatable behaviour that is consistent with updatable.
>
> It's not correct. "truncatable" option works the same as "updatable".
> I've confirmed that the table can be truncated with the combination:
> truncatable on the server setting is false & truncatable on the table
> setting is true.
> I've also added to the test.

Yeah you are right! I was wrong in understanding.

> > 7) Did you try checking whether we reach hash_destroy code when a
> > failure happens on executing truncate on a remote table? Otherwise we
> > might want to do routine->ExecForeignTruncate inside PG_TRY block?
>
> I've added PG_TRY block.

Did you check that hash_destroy is not reachable when an error occurs
on the remote server while executing truncate command? If yes, then
only we will have PG_TRY block, otherwise not.

> > 9) It will be good if you can divide up your patch into 3 separate
> > patches - 0001 code, 0002 tests, 0003 documentation
>
> I'll do it when I send a patch in the future, please forgive me on this patch.

That's okay.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

> Did you check that hash_destroy is not reachable when an error occurs
> on the remote server while executing truncate command?

I've checked it and hash_destroy doesn't work on error.

I just adding elog() to check this:
+ elog(NOTICE,"destroyed");
+ hash_destroy(ft_htab);

Then I've checked by the test.

+ -- 'truncatable' option
+ ALTER SERVER loopback OPTIONS (ADD truncatable 'false');
+ TRUNCATE tru_ftable; -- error
+ ERROR:  truncate on "tru_ftable" is prohibited
<-   hash_destroy doesn't work.
+ ALTER FOREIGN TABLE tru_ftable OPTIONS (ADD truncatable 'true');
+ TRUNCATE tru_ftable; -- accepted
+ NOTICE:  destroyed     <-  hash_destroy works.

Of course, the elog() is not included in v13 patch.

2021年4月5日(月) 23:35 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Mon, Apr 5, 2021 at 7:38 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > > 3) I think we need truncatable behaviour that is consistent with updatable.
> >
> > It's not correct. "truncatable" option works the same as "updatable".
> > I've confirmed that the table can be truncated with the combination:
> > truncatable on the server setting is false & truncatable on the table
> > setting is true.
> > I've also added to the test.
>
> Yeah you are right! I was wrong in understanding.
>
> > > 7) Did you try checking whether we reach hash_destroy code when a
> > > failure happens on executing truncate on a remote table? Otherwise we
> > > might want to do routine->ExecForeignTruncate inside PG_TRY block?
> >
> > I've added PG_TRY block.
>
> Did you check that hash_destroy is not reachable when an error occurs
> on the remote server while executing truncate command? If yes, then
> only we will have PG_TRY block, otherwise not.
>
> > > 9) It will be good if you can divide up your patch into 3 separate
> > > patches - 0001 code, 0002 tests, 0003 documentation
> >
> > I'll do it when I send a patch in the future, please forgive me on this patch.
>
> That's okay.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> > Did you check that hash_destroy is not reachable when an error occurs
> > on the remote server while executing truncate command?
>
> I've checked it and hash_destroy doesn't work on error.
>
> I just adding elog() to check this:
> + elog(NOTICE,"destroyed");
> + hash_destroy(ft_htab);
>
> Then I've checked by the test.
>
> + -- 'truncatable' option
> + ALTER SERVER loopback OPTIONS (ADD truncatable 'false');
> + TRUNCATE tru_ftable; -- error
> + ERROR:  truncate on "tru_ftable" is prohibited
> <-   hash_destroy doesn't work.
> + ALTER FOREIGN TABLE tru_ftable OPTIONS (ADD truncatable 'true');
> + TRUNCATE tru_ftable; -- accepted
> + NOTICE:  destroyed     <-  hash_destroy works.
>
> Of course, the elog() is not included in v13 patch.

Few more comments on v13:

1) Are we using all of these macros? I see that we are setting them
but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
them?
+#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
+#define TRUNCATE_REL_CONTEXT_ONLY         0x02
+#define TRUNCATE_REL_CONTEXT_CASCADING     0x04

2) Why is this change for? The existing comment properly says the
behaviour i.e. all foreign tables are updatable by default.
@@ -2216,7 +2223,7 @@ postgresIsForeignRelUpdatable(Relation rel)
     ListCell   *lc;

     /*
-     * By default, all postgres_fdw foreign tables are assumed updatable. This
+     * By default, all postgres_fdw foreign tables are assumed NOT
truncatable. This

And the below comment is wrong, by default foreign tables are assumed
truncatable.
+     * By default, all postgres_fdw foreign tables are NOT assumed
truncatable. This
+     * can be overridden by a per-server setting, which in turn can be
+     * overridden by a per-table setting.
+     */

3) In the docs, let's not combine updatable and truncatable together.
Have a separate section for <title>Truncatability Options</title>, all
the documentation related to it be under this new section.
    <para>
     By default all foreign tables using
<filename>postgres_fdw</filename> are assumed
-    to be updatable.  This may be overridden using the following option:
+    to be updatable and truncatable.  This may be overridden using
the following options:
    </para>

4) I have a basic question: If I have a truncate statement with a mix
of local and foreign tables, IIUC, the patch is dividing up a single
truncate statement into two truncate local tables, truncate foreign
tables. Is this transaction safe at all?
A better illustration: TRUNCATE local_rel1, local_rel2, local_rel3,
foreign_rel1, foreign_rel2, foreign_rel3;
Your patch executes TRUNCATE local_rel1, local_rel2, local_rel3; on
local server and TRUNCATE foreign_rel1, foreign_rel2, foreign_rel3; on
remote server. Am I right?
Now the question is: if any failure occurs either in local server
execution or in remote server execution, the other truncate command
would succeed right? Isn't this non-transactional and we are breaking
the transactional guarantee of the truncation.
Looks like the order of execution is - first local rel truncation and
then foreign rel truncation, so what happens if foreign rel truncation
fails? Can we revert the local rel truncation?

6) Again v13 has white space errors, please ensure to run git diff
--check on every patch.
bharath@ubuntu:~/workspace/postgres$ git apply
/mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch
/mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:41:
trailing whitespace.
/mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:47:
trailing whitespace.

warning: 2 lines add whitespace errors.
bharath@ubuntu:~/workspace/postgres$ git diff --check
contrib/postgres_fdw/deparse.c:2200: trailing whitespace.
+
contrib/postgres_fdw/deparse.c:2206: trailing whitespace.
+

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

Thank you for checking v13, and here is v14 patch.

> 1) Are we using all of these macros? I see that we are setting them
> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> them?

These may be needed for the foreign data handler other than postgres_fdw.

> 2) Why is this change for? The existing comment properly says the
> behaviour i.e. all foreign tables are updatable by default.

This is just a mistake. I've fixed it.

> 3) In the docs, let's not combine updatable and truncatable together.
> Have a separate section for <title>Truncatability Options</title>, all
> the documentation related to it be under this new section.

Sure. I've added new section.

> 4) I have a basic question: If I have a truncate statement with a mix
> of local and foreign tables, IIUC, the patch is dividing up a single
> truncate statement into two truncate local tables, truncate foreign
> tables. Is this transaction safe at all?

According to this discussion, we can revert both tables in the local
and the server.
https://www.postgresql.org/message-id/CAOP8fzbuJ5GdKa%2B%3DGtizbqFtO2xsQbn4mVjjzunmsNVJMChSMQ%40mail.gmail.com

> 6) Again v13 has white space errors, please ensure to run git diff
> --check on every patch.

Umm..  I'm sure I've checked it on v13.
I've confirmed it on v14.

2021年4月6日(火) 13:33 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> >
> > > Did you check that hash_destroy is not reachable when an error occurs
> > > on the remote server while executing truncate command?
> >
> > I've checked it and hash_destroy doesn't work on error.
> >
> > I just adding elog() to check this:
> > + elog(NOTICE,"destroyed");
> > + hash_destroy(ft_htab);
> >
> > Then I've checked by the test.
> >
> > + -- 'truncatable' option
> > + ALTER SERVER loopback OPTIONS (ADD truncatable 'false');
> > + TRUNCATE tru_ftable; -- error
> > + ERROR:  truncate on "tru_ftable" is prohibited
> > <-   hash_destroy doesn't work.
> > + ALTER FOREIGN TABLE tru_ftable OPTIONS (ADD truncatable 'true');
> > + TRUNCATE tru_ftable; -- accepted
> > + NOTICE:  destroyed     <-  hash_destroy works.
> >
> > Of course, the elog() is not included in v13 patch.
>
> Few more comments on v13:
>
> 1) Are we using all of these macros? I see that we are setting them
> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> them?
> +#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
> +#define TRUNCATE_REL_CONTEXT_ONLY         0x02
> +#define TRUNCATE_REL_CONTEXT_CASCADING     0x04
>
> 2) Why is this change for? The existing comment properly says the
> behaviour i.e. all foreign tables are updatable by default.
> @@ -2216,7 +2223,7 @@ postgresIsForeignRelUpdatable(Relation rel)
>      ListCell   *lc;
>
>      /*
> -     * By default, all postgres_fdw foreign tables are assumed updatable. This
> +     * By default, all postgres_fdw foreign tables are assumed NOT
> truncatable. This
>
> And the below comment is wrong, by default foreign tables are assumed
> truncatable.
> +     * By default, all postgres_fdw foreign tables are NOT assumed
> truncatable. This
> +     * can be overridden by a per-server setting, which in turn can be
> +     * overridden by a per-table setting.
> +     */
>
> 3) In the docs, let's not combine updatable and truncatable together.
> Have a separate section for <title>Truncatability Options</title>, all
> the documentation related to it be under this new section.
>     <para>
>      By default all foreign tables using
> <filename>postgres_fdw</filename> are assumed
> -    to be updatable.  This may be overridden using the following option:
> +    to be updatable and truncatable.  This may be overridden using
> the following options:
>     </para>
>
> 4) I have a basic question: If I have a truncate statement with a mix
> of local and foreign tables, IIUC, the patch is dividing up a single
> truncate statement into two truncate local tables, truncate foreign
> tables. Is this transaction safe at all?
> A better illustration: TRUNCATE local_rel1, local_rel2, local_rel3,
> foreign_rel1, foreign_rel2, foreign_rel3;
> Your patch executes TRUNCATE local_rel1, local_rel2, local_rel3; on
> local server and TRUNCATE foreign_rel1, foreign_rel2, foreign_rel3; on
> remote server. Am I right?
> Now the question is: if any failure occurs either in local server
> execution or in remote server execution, the other truncate command
> would succeed right? Isn't this non-transactional and we are breaking
> the transactional guarantee of the truncation.
> Looks like the order of execution is - first local rel truncation and
> then foreign rel truncation, so what happens if foreign rel truncation
> fails? Can we revert the local rel truncation?
>
> 6) Again v13 has white space errors, please ensure to run git diff
> --check on every patch.
> bharath@ubuntu:~/workspace/postgres$ git apply
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:41:
> trailing whitespace.
> /mnt/hgfs/Shared/pgsql14-truncate-on-foreign-table.v13.patch:47:
> trailing whitespace.
>
> warning: 2 lines add whitespace errors.
> bharath@ubuntu:~/workspace/postgres$ git diff --check
> contrib/postgres_fdw/deparse.c:2200: trailing whitespace.
> +
> contrib/postgres_fdw/deparse.c:2206: trailing whitespace.
> +
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> Thank you for checking v13, and here is v14 patch.
>
> > 1) Are we using all of these macros? I see that we are setting them
> > but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> > them?
>
> These may be needed for the foreign data handler other than postgres_fdw.

I'm not sure about this, but if it's discussed upthread and agreed
upon, I'm fine with it.

> > 4) I have a basic question: If I have a truncate statement with a mix
> > of local and foreign tables, IIUC, the patch is dividing up a single
> > truncate statement into two truncate local tables, truncate foreign
> > tables. Is this transaction safe at all?
>
> According to this discussion, we can revert both tables in the local
> and the server.
> https://www.postgresql.org/message-id/CAOP8fzbuJ5GdKa%2B%3DGtizbqFtO2xsQbn4mVjjzunmsNVJMChSMQ%40mail.gmail.com

On giving more thought on this, it looks like we are safe i.e. local
truncation will get reverted. Because if an error occurs on foreign
table truncation, the control in the local server would go to
pgfdw_report_error which generates an error in the local server which
aborts the local transaction and so the local table truncations would
get reverted.

+    /* run remote query */
+    if (!PQsendQuery(conn, sql.data))
+        pgfdw_report_error(ERROR, NULL, conn, false, sql.data);
+
+    res = pgfdw_get_result(conn, sql.data);
+
+    if (PQresultStatus(res) != PGRES_COMMAND_OK)
+        pgfdw_report_error(ERROR, res, conn, true, sql.data);

I still feel that the above bunch of code is duplicate of what
do_sql_command function already has. I would recommend that we just
make that function non-static(it's easy to do) and keep the
declaration in postgres_fdw.h and use it in the
postgresExecForeignTruncate.

Another minor comment:
We could move +    ForeignServer  *serv = NULL; within foreach (lc,
frels_list), because it's not being used outside.

The v14 patch mostly looks good to me other than the above comments.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
>
> Thank you for checking v13, and here is v14 patch.

cfbot failure on v14 - https://cirrus-ci.com/task/4772360931770368.
Looks like it is not related to this patch, please re-confirm it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

I've attached v15.

> I still feel that the above bunch of code is duplicate of what
> do_sql_command function already has. I would recommend that we just
> make that function non-static(it's easy to do) and keep the
> declaration in postgres_fdw.h and use it in the
> postgresExecForeignTruncate.

I've tried this on v15.

> Another minor comment:
> We could move +    ForeignServer  *serv = NULL; within foreach (lc,
> frels_list), because it's not being used outside.

I've moved it.

> cfbot failure on v14 - https://cirrus-ci.com/task/4772360931770368.
> Looks like it is not related to this patch, please re-confirm it.

I've checked v15 patch with "make check-world" and confirmed this passed.



2021年4月6日(火) 23:25 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> >
> > Thank you for checking v13, and here is v14 patch.
>
> cfbot failure on v14 - https://cirrus-ci.com/task/4772360931770368.
> Looks like it is not related to this patch, please re-confirm it.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 6, 2021 at 10:15 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> I've checked v15 patch with "make check-world" and confirmed this passed.

Thanks for the patch. One minor thing - I think "mixtured" is not the
correct word in "+-- partition table mixtured by table and foreign
table". How about something like "+-- partitioned table with both
local and foreign table as partitions"?

The v15 patch basically looks good to me. I have no more comments.

CF entry https://commitfest.postgresql.org/32/2972/ still says it's
"waiting on author", do you want to change it to "needs review" if you
have no open points left so that others can take a look at it?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

> One minor thing - I think "mixtured" is not the
> correct word in "+-- partition table mixtured by table and foreign
> table". How about something like "+-- partitioned table with both
> local and foreign table as partitions"?

Sure. I've fixed this.

> The v15 patch basically looks good to me. I have no more comments.
Thank you for checking this many times.

> CF entry https://commitfest.postgresql.org/32/2972/ still says it's
> "waiting on author", do you want to change it to "needs review" if you
> have no open points left so that others can take a look at it?
Yes, please.

2021年4月7日(水) 10:15 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Tue, Apr 6, 2021 at 10:15 PM Kazutaka Onishi <onishi@heterodb.com> wrote:
> > I've checked v15 patch with "make check-world" and confirmed this passed.
>
> Thanks for the patch. One minor thing - I think "mixtured" is not the
> correct word in "+-- partition table mixtured by table and foreign
> table". How about something like "+-- partitioned table with both
> local and foreign table as partitions"?
>
> The v15 patch basically looks good to me. I have no more comments.
>
> CF entry https://commitfest.postgresql.org/32/2972/ still says it's
> "waiting on author", do you want to change it to "needs review" if you
> have no open points left so that others can take a look at it?
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com

On 2021/04/06 21:06, Kazutaka Onishi wrote:
> Thank you for checking v13, and here is v14 patch.
> 
>> 1) Are we using all of these macros? I see that we are setting them
>> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
>> them?
> 
> These may be needed for the foreign data handler other than postgres_fdw.

Could you tell me how such FDWs use TRUNCATE_REL_CONTEXT_CASCADING and _NORMAL? I'm still not sure if
TRUNCATE_REL_CONTEXT_CASCADINGis really required.
 

With the patch, both inherited and referencing relations are marked as TRUNCATE_REL_CONTEXT_CASCADING? Is this ok for
thatuse? Or we should distinguish them?
 

+#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
+#define TRUNCATE_REL_CONTEXT_ONLY         0x02
+#define TRUNCATE_REL_CONTEXT_CASCADING     0x04

With the patch, these are defined as flag bits. But ExecuteTruncate() seems to always set the entry in relids_extra to
eitherof them, not the combination of them. So we can define them as enum?
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月8日(木) 4:19 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/06 21:06, Kazutaka Onishi wrote:
> > Thank you for checking v13, and here is v14 patch.
> >
> >> 1) Are we using all of these macros? I see that we are setting them
> >> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> >> them?
> >
> > These may be needed for the foreign data handler other than postgres_fdw.
>
> Could you tell me how such FDWs use TRUNCATE_REL_CONTEXT_CASCADING and _NORMAL? I'm still not sure if
TRUNCATE_REL_CONTEXT_CASCADINGis really required. 
>
https://www.postgresql.org/message-id/20200102144644.GM3195%40tamriel.snowman.net

This is the suggestion when I added the flag to inform cascading.

| .... Instead, I'd suggest we have the core code build
| up a list of tables to truncate, for each server, based just on the list
| passed in by the user, and then also pass in if CASCADE was included or
| not, and then let the FDW handle that in whatever way makes sense for
| the foreign server (which, for a PG system, would probably be just
| building up the TRUNCATE command and running it with or without the
| CASCADE option, but it might be different on other systems).
|
Indeed, it is not a strong technical reason at this moment.
(And, I also don't have idea to distinct these differences in my module also.)

> With the patch, both inherited and referencing relations are marked as TRUNCATE_REL_CONTEXT_CASCADING? Is this ok for
thatuse? Or we should distinguish them? 
>
In addition, even though my prior implementation distinguished and deliver
the status whether the truncate command is issued with NORMAL or ONLY,
does the remote query by postgres_fdw needs to follow the manner?

Please assume the case when a foreign-table "ft" that maps a remote table
with some child-relations.
If we run TRUNCATE ONLY ft at the local server, postgres_fdw setup
a remote truncate command with "ONLY" qualifier, then remote postgresql
server truncate only parent table of the remote side.
Next, "SELECT * FROM ft" command returns some valid rows from the
child tables in the remote side, even if it is just after TRUNCATE command.
Is it a intuitive behavior for users?

Even though we have discussed about the flags and expected behavior of
foreign truncate, strip of the relids_extra may be the most straight-forward
API design.
So, in other words, the API requires FDW driver to make the entire data
represented by the foreign table empty, by ExecForeignTruncate().
It is probably more consistent to look at DropBehavior for listing-up the
target relations at the local relations only.

How about your thought?

If we stand on the above design, ExecForeignTruncate() don't needs
frels_extra and behavior arguments.

> +#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
> +#define TRUNCATE_REL_CONTEXT_ONLY         0x02
> +#define TRUNCATE_REL_CONTEXT_CASCADING     0x04
>
> With the patch, these are defined as flag bits. But ExecuteTruncate() seems to always set the entry in relids_extra
toeither of them, not the combination of them. So we can define them as enum? 
>
Regardless of my above comment, It's a bug.
When list_member_oid(relids, myrelid) == true, we have to set proper flag on the
relevant frels_extra member, not just ignoring.

Best regards,


Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/08 10:56, Kohei KaiGai wrote:
> 2021年4月8日(木) 4:19 Fujii Masao <masao.fujii@oss.nttdata.com>:
>>
>> On 2021/04/06 21:06, Kazutaka Onishi wrote:
>>> Thank you for checking v13, and here is v14 patch.
>>>
>>>> 1) Are we using all of these macros? I see that we are setting them
>>>> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
>>>> them?
>>>
>>> These may be needed for the foreign data handler other than postgres_fdw.
>>
>> Could you tell me how such FDWs use TRUNCATE_REL_CONTEXT_CASCADING and _NORMAL? I'm still not sure if
TRUNCATE_REL_CONTEXT_CASCADINGis really required.
 
>>
> https://www.postgresql.org/message-id/20200102144644.GM3195%40tamriel.snowman.net
> 
> This is the suggestion when I added the flag to inform cascading.
> 
> | .... Instead, I'd suggest we have the core code build
> | up a list of tables to truncate, for each server, based just on the list
> | passed in by the user, and then also pass in if CASCADE was included or
> | not, and then let the FDW handle that in whatever way makes sense for
> | the foreign server (which, for a PG system, would probably be just
> | building up the TRUNCATE command and running it with or without the
> | CASCADE option, but it might be different on other systems).
> |
> Indeed, it is not a strong technical reason at this moment.
> (And, I also don't have idea to distinct these differences in my module also.)

CASCADE option mentioned in the above seems the CASCADE clause specified in TRUNCATE command. No? So the above doesn't
seemto suggest to include the information about how each table to truncate is picked up. Am I missing something?
 


> 
>> With the patch, both inherited and referencing relations are marked as TRUNCATE_REL_CONTEXT_CASCADING? Is this ok
forthat use? Or we should distinguish them?
 
>>
> In addition, even though my prior implementation distinguished and deliver
> the status whether the truncate command is issued with NORMAL or ONLY,
> does the remote query by postgres_fdw needs to follow the manner?
> 
> Please assume the case when a foreign-table "ft" that maps a remote table
> with some child-relations.
> If we run TRUNCATE ONLY ft at the local server, postgres_fdw setup
> a remote truncate command with "ONLY" qualifier, then remote postgresql
> server truncate only parent table of the remote side.
> Next, "SELECT * FROM ft" command returns some valid rows from the
> child tables in the remote side, even if it is just after TRUNCATE command.
> Is it a intuitive behavior for users?

Yes, because that's the same behavior as for the local tables. No?

If this understanding is true, the following note that the patch added is also intuitive, and not necessary? At least
"partitionleafs" part should be removed because TRUNCATE ONLY fails if the remote table is a partitioned table.
 

+       Pay attention for the case when a foreign table maps remote table
+       that has inherited children or partition leafs.
+       <command>TRUNCATE</command> specifies the foreign tables with
+       <literal>ONLY</literal> clause, remove queries over the
+       <filename>postgres_fdw</filename> also specify remote tables with
+       <literal>ONLY</literal> clause, that will truncate only parent
+       portion of the remote table. In the results, it looks like
+       <command>TRUNCATE</command> command partially eliminated contents
+       of the foreign tables.


> 
> Even though we have discussed about the flags and expected behavior of
> foreign truncate, strip of the relids_extra may be the most straight-forward
> API design.
> So, in other words, the API requires FDW driver to make the entire data
> represented by the foreign table empty, by ExecForeignTruncate().
> It is probably more consistent to look at DropBehavior for listing-up the
> target relations at the local relations only.
> 
> How about your thought?

I was thinking to remove only TRUNCATE_REL_CONTEXT_CASCADING if that's really not necessary. That is, rels_extra is
stillused to indicate whether each table is specified with ONLY option or not. To do this, we can use _NORMAL and
_ONLY.Or we can also make that as the list of boolean flag (indicating whether ONLY is specified or not).
 


> 
> If we stand on the above design, ExecForeignTruncate() don't needs
> frels_extra and behavior arguments.
> 
>> +#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
>> +#define TRUNCATE_REL_CONTEXT_ONLY         0x02
>> +#define TRUNCATE_REL_CONTEXT_CASCADING     0x04
>>
>> With the patch, these are defined as flag bits. But ExecuteTruncate() seems to always set the entry in relids_extra
toeither of them, not the combination of them. So we can define them as enum?
 
>>
> Regardless of my above comment, It's a bug.
> When list_member_oid(relids, myrelid) == true, we have to set proper flag on the
> relevant frels_extra member, not just ignoring.

One concern about this is that local tables are not processed that way. For local tables, the information (whether ONLY
isspecified or not) of the table found first is used. For example, when we execute "TRUNCATE ONLY tbl, tbl" and
"TRUNCATEtbl, ONLY tbl", the former truncates only parent table because "ONLY tbl" is found first. But the latter
truncatesthe parent and all inherited tables because "tbl" is found first.
 

If even foreign table follows this manner, current patch's logic seems right.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月8日(木) 11:44 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/08 10:56, Kohei KaiGai wrote:
> > 2021年4月8日(木) 4:19 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >>
> >> On 2021/04/06 21:06, Kazutaka Onishi wrote:
> >>> Thank you for checking v13, and here is v14 patch.
> >>>
> >>>> 1) Are we using all of these macros? I see that we are setting them
> >>>> but we only use TRUNCATE_REL_CONTEXT_ONLY. If not used, can we remove
> >>>> them?
> >>>
> >>> These may be needed for the foreign data handler other than postgres_fdw.
> >>
> >> Could you tell me how such FDWs use TRUNCATE_REL_CONTEXT_CASCADING and _NORMAL? I'm still not sure if
TRUNCATE_REL_CONTEXT_CASCADINGis really required. 
> >>
> > https://www.postgresql.org/message-id/20200102144644.GM3195%40tamriel.snowman.net
> >
> > This is the suggestion when I added the flag to inform cascading.
> >
> > | .... Instead, I'd suggest we have the core code build
> > | up a list of tables to truncate, for each server, based just on the list
> > | passed in by the user, and then also pass in if CASCADE was included or
> > | not, and then let the FDW handle that in whatever way makes sense for
> > | the foreign server (which, for a PG system, would probably be just
> > | building up the TRUNCATE command and running it with or without the
> > | CASCADE option, but it might be different on other systems).
> > |
> > Indeed, it is not a strong technical reason at this moment.
> > (And, I also don't have idea to distinct these differences in my module also.)
>
> CASCADE option mentioned in the above seems the CASCADE clause specified in TRUNCATE command. No? So the above
doesn'tseem to suggest to include the information about how each table to truncate is picked up. Am I missing
something?
>
It might be a bit different context.

> >
> >> With the patch, both inherited and referencing relations are marked as TRUNCATE_REL_CONTEXT_CASCADING? Is this ok
forthat use? Or we should distinguish them? 
> >>
> > In addition, even though my prior implementation distinguished and deliver
> > the status whether the truncate command is issued with NORMAL or ONLY,
> > does the remote query by postgres_fdw needs to follow the manner?
> >
> > Please assume the case when a foreign-table "ft" that maps a remote table
> > with some child-relations.
> > If we run TRUNCATE ONLY ft at the local server, postgres_fdw setup
> > a remote truncate command with "ONLY" qualifier, then remote postgresql
> > server truncate only parent table of the remote side.
> > Next, "SELECT * FROM ft" command returns some valid rows from the
> > child tables in the remote side, even if it is just after TRUNCATE command.
> > Is it a intuitive behavior for users?
>
> Yes, because that's the same behavior as for the local tables. No?
>
No. ;-p

When we define a foreign-table as follows,

postgres=# CREATE FOREIGN TABLE ft (id int, v text)
                   SERVER loopback OPTIONS (table_name 't_parent',
truncatable 'true');
postgres=# select * from ft;
 id |         v
----+-------------------
  1 | 1 in the parent
  2 | 2 in the parent
  3 | 3 in the parent
  4 | 4 in the parent
 11 | 11 in the child_1
 12 | 12 in the child_1
 13 | 13 in the child_1
 21 | 21 in the child_2
 22 | 22 in the child_2
 23 | 23 in the child_2
(10 rows)

TRUNCATE ONLY eliminates the rows come from parent table on the remote side,
even though this foreign table has no parent-child relationship in the
local side.

postgres=# begin;
BEGIN
postgres=# truncate only ft;
TRUNCATE TABLE
postgres=# select * from ft;
 id |         v
----+-------------------
 11 | 11 in the child_1
 12 | 12 in the child_1
 13 | 13 in the child_1
 21 | 21 in the child_2
 22 | 22 in the child_2
 23 | 23 in the child_2
(6 rows)

postgres=# abort;
ROLLBACK

In case when a local table (with no children) has same contents,
TRUNCATE command
witll remove the entire table contents.

postgres=# select * INTO tt FROM ft;
SELECT 10
postgres=# select * from tt;
 id |         v
----+-------------------
  1 | 1 in the parent
  2 | 2 in the parent
  3 | 3 in the parent
  4 | 4 in the parent
 11 | 11 in the child_1
 12 | 12 in the child_1
 13 | 13 in the child_1
 21 | 21 in the child_2
 22 | 22 in the child_2
 23 | 23 in the child_2
(10 rows)

postgres=# truncate only tt;
TRUNCATE TABLE
postgres=# select * from tt;
 id | v
----+---
(0 rows)

> If this understanding is true, the following note that the patch added is also intuitive, and not necessary? At least
"partitionleafs" part should be removed because TRUNCATE ONLY fails if the remote table is a partitioned table. 
>
> +       Pay attention for the case when a foreign table maps remote table
> +       that has inherited children or partition leafs.
> +       <command>TRUNCATE</command> specifies the foreign tables with
> +       <literal>ONLY</literal> clause, remove queries over the
> +       <filename>postgres_fdw</filename> also specify remote tables with
> +       <literal>ONLY</literal> clause, that will truncate only parent
> +       portion of the remote table. In the results, it looks like
> +       <command>TRUNCATE</command> command partially eliminated contents
> +       of the foreign tables.
>
Base on the above assumption, I don't think it should be a part of
documentation.
On the other hands, we need to describe this API requires FDW driver to wipe out
the entire data on behalf of the foreign tables once they are picked up by the
ExecuteTruncate().

> > Even though we have discussed about the flags and expected behavior of
> > foreign truncate, strip of the relids_extra may be the most straight-forward
> > API design.
> > So, in other words, the API requires FDW driver to make the entire data
> > represented by the foreign table empty, by ExecForeignTruncate().
> > It is probably more consistent to look at DropBehavior for listing-up the
> > target relations at the local relations only.
> >
> > How about your thought?
>
> I was thinking to remove only TRUNCATE_REL_CONTEXT_CASCADING if that's really not necessary. That is, rels_extra is
stillused to indicate whether each table is specified with ONLY option or not. To do this, we can use _NORMAL and
_ONLY.Or we can also make that as the list of boolean flag (indicating whether ONLY is specified or not). 
>
I'm inclined to eliminate relids_extra list itself, because FDW
drivers don't need to
distinguish the CASCADING, NORMAL or ONLY cases.
The ExecForeignTruncate receives a list of foreign tables that is already
expanded by the ExecuteTruncate(), thus, all the FDW driver shall do is
just wipe out entire data mapped to the individual foreign tables
Also, FDW driver don't need to know DropBehavior.

> > If we stand on the above design, ExecForeignTruncate() don't needs
> > frels_extra and behavior arguments.
> >
> >> +#define TRUNCATE_REL_CONTEXT_NORMAL       0x01
> >> +#define TRUNCATE_REL_CONTEXT_ONLY         0x02
> >> +#define TRUNCATE_REL_CONTEXT_CASCADING     0x04
> >>
> >> With the patch, these are defined as flag bits. But ExecuteTruncate() seems to always set the entry in
relids_extrato either of them, not the combination of them. So we can define them as enum? 
> >>
> > Regardless of my above comment, It's a bug.
> > When list_member_oid(relids, myrelid) == true, we have to set proper flag on the
> > relevant frels_extra member, not just ignoring.
>
> One concern about this is that local tables are not processed that way. For local tables, the information (whether
ONLYis specified or not) of the table found first is used. For example, when we execute "TRUNCATE ONLY tbl, tbl" and
"TRUNCATEtbl, ONLY tbl", the former truncates only parent table because "ONLY tbl" is found first. But the latter
truncatesthe parent and all inherited tables because "tbl" is found first. 
>
> If even foreign table follows this manner, current patch's logic seems right.
>
-1. :-(
It should be fixed, even if we try to deliver the relids_extra list.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/08 13:43, Kohei KaiGai wrote:
> In case when a local table (with no children) has same contents,
> TRUNCATE command
> witll remove the entire table contents.

But if there are local child tables that inherit the local parent table, and TRUNCATE ONLY <parent table> is executed,
onlythe contents in the parent will be truncated. I was thinking that this behavior should be applied to the foreign
tablewhose remote (parent) table have remote child tables.
 

So what we need to reach the consensus is; how far ONLY option affects. Please imagine the case where we have

(1) local parent table, also foreign table of remote parent table
(2) local child table, inherits local parent table
(3) remote parent table
(4) remote child table, inherits remote parent table

I think that we agree all (1), (2), (3) and (4) should be truncated if local parent table (1) is specified without ONLY
inTRUNCATE command. OTOH, if ONLY is specified, we agree that at least local child table (2) should NOT be truncated.
 

So the remaining point is; remote tables (3) and (4) should be truncated or not when ONLY is specified? You seem to
arguethat both should be truncated by removing extra list. I was thinking that only remote parent table (3) should be
truncated.That is, IMO we should treat the truncation on foreign table as the same as that on its forein data source.
 

Other people might think neither (3) nor (4) should be truncated in that case because ONLY should affect only the table
directlyspecified in TRUNCATE command, i.e., local parent table (1). For now this also looks good to me.
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月8日(木) 15:04 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/08 13:43, Kohei KaiGai wrote:
> > In case when a local table (with no children) has same contents,
> > TRUNCATE command
> > witll remove the entire table contents.
>
> But if there are local child tables that inherit the local parent table, and TRUNCATE ONLY <parent table> is
executed,only the contents in the parent will be truncated. I was thinking that this behavior should be applied to the
foreigntable whose remote (parent) table have remote child tables. 
>
> So what we need to reach the consensus is; how far ONLY option affects. Please imagine the case where we have
>
> (1) local parent table, also foreign table of remote parent table
> (2) local child table, inherits local parent table
> (3) remote parent table
> (4) remote child table, inherits remote parent table
>
> I think that we agree all (1), (2), (3) and (4) should be truncated if local parent table (1) is specified without
ONLYin TRUNCATE command. OTOH, if ONLY is specified, we agree that at least local child table (2) should NOT be
truncated.
>
My understanding of a foreign table is a representation of external
data, including remote RDBMS but not only RDBMS,
regardless of the parent-child relationship at the local side.
So, once a local foreign table wraps entire tables tree (a parent and
relevant children) at the remote side, at least, it shall
be considered as a unified data chunk from the standpoint of the local side.

Please assume if file_fdw could map 3 different CSV files, then
truncate on the foreign table may eliminate just 1 of 3 files.
Is it an expected / preferable behavior?
Basically, we don't assume any charasteristics of the data on behalf
of the FDW driver, even if it is PostgreSQL server.
Thus, I think the new API will expect  to eliminate the entire rows on
behalf of the foreign table, regardless of the ONLY-clause,
because it already controls which foreign-tables shall be picked up,
but does not control which part of the foreign table
shall be eliminated.

> So the remaining point is; remote tables (3) and (4) should be truncated or not when ONLY is specified? You seem to
arguethat both should be truncated by removing extra list. I was thinking that only remote parent table (3) should be
truncated.That is, IMO we should treat the truncation on foreign table as the same as that on its forein data source. 
>
> Other people might think neither (3) nor (4) should be truncated in that case because ONLY should affect only the
tabledirectly specified in TRUNCATE command, i.e., local parent table (1). For now this also looks good to me. 
>
In case when the local foreign table is a parent, the entire remote
table shall be truncated, if ONLY is given.
In case when the local foreign table is a child, nothing shall be
happen (API is not called), if ONLY is given.

IMO, it is stable and simple definition, even if FDW driver wraps
non-RDBMS data source that has no idea
of table inheritance.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/08 15:48, Kohei KaiGai wrote:
> 2021年4月8日(木) 15:04 Fujii Masao <masao.fujii@oss.nttdata.com>:
>>
>> On 2021/04/08 13:43, Kohei KaiGai wrote:
>>> In case when a local table (with no children) has same contents,
>>> TRUNCATE command
>>> witll remove the entire table contents.
>>
>> But if there are local child tables that inherit the local parent table, and TRUNCATE ONLY <parent table> is
executed,only the contents in the parent will be truncated. I was thinking that this behavior should be applied to the
foreigntable whose remote (parent) table have remote child tables.
 
>>
>> So what we need to reach the consensus is; how far ONLY option affects. Please imagine the case where we have
>>
>> (1) local parent table, also foreign table of remote parent table
>> (2) local child table, inherits local parent table
>> (3) remote parent table
>> (4) remote child table, inherits remote parent table
>>
>> I think that we agree all (1), (2), (3) and (4) should be truncated if local parent table (1) is specified without
ONLYin TRUNCATE command. OTOH, if ONLY is specified, we agree that at least local child table (2) should NOT be
truncated.
>>
> My understanding of a foreign table is a representation of external
> data, including remote RDBMS but not only RDBMS,
> regardless of the parent-child relationship at the local side.
> So, once a local foreign table wraps entire tables tree (a parent and
> relevant children) at the remote side, at least, it shall
> be considered as a unified data chunk from the standpoint of the local side.

At least for me it's not intuitive to truncate the remote table and its all dependent tables even though users
explicitlyspecify ONLY for the foreign table. As far as I read the past discussion, some people was thinking the same.
 

> 
> Please assume if file_fdw could map 3 different CSV files, then
> truncate on the foreign table may eliminate just 1 of 3 files.
> Is it an expected / preferable behavior?

I think that's up to each FDW. That is, IMO the information about whether ONLY is specified or not for each table
shouldbe passed to FDW. Then FDW itself should determine how to handle that information.
 

Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That is,
extralist for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary.
 

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/08 18:25, Fujii Masao wrote:
> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary.
 

The patch failed to be applied because of recent commit.
Attached is the updated version of the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月8日(木) 18:25 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/08 15:48, Kohei KaiGai wrote:
> > 2021年4月8日(木) 15:04 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >>
> >> On 2021/04/08 13:43, Kohei KaiGai wrote:
> >>> In case when a local table (with no children) has same contents,
> >>> TRUNCATE command
> >>> witll remove the entire table contents.
> >>
> >> But if there are local child tables that inherit the local parent table, and TRUNCATE ONLY <parent table> is
executed,only the contents in the parent will be truncated. I was thinking that this behavior should be applied to the
foreigntable whose remote (parent) table have remote child tables. 
> >>
> >> So what we need to reach the consensus is; how far ONLY option affects. Please imagine the case where we have
> >>
> >> (1) local parent table, also foreign table of remote parent table
> >> (2) local child table, inherits local parent table
> >> (3) remote parent table
> >> (4) remote child table, inherits remote parent table
> >>
> >> I think that we agree all (1), (2), (3) and (4) should be truncated if local parent table (1) is specified without
ONLYin TRUNCATE command. OTOH, if ONLY is specified, we agree that at least local child table (2) should NOT be
truncated.
> >>
> > My understanding of a foreign table is a representation of external
> > data, including remote RDBMS but not only RDBMS,
> > regardless of the parent-child relationship at the local side.
> > So, once a local foreign table wraps entire tables tree (a parent and
> > relevant children) at the remote side, at least, it shall
> > be considered as a unified data chunk from the standpoint of the local side.
>
> At least for me it's not intuitive to truncate the remote table and its all dependent tables even though users
explicitlyspecify ONLY for the foreign table. As far as I read the past discussion, some people was thinking the same. 
>
> >
> > Please assume if file_fdw could map 3 different CSV files, then
> > truncate on the foreign table may eliminate just 1 of 3 files.
> > Is it an expected / preferable behavior?
>
> I think that's up to each FDW. That is, IMO the information about whether ONLY is specified or not for each table
shouldbe passed to FDW. Then FDW itself should determine how to handle that information. 
>
> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary. 
>
Ok, it's fair enought for me.

I'll try to sort out my thought, then raise a follow-up discussion if necessary.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/08 22:02, Kohei KaiGai wrote:
>> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary.
 

Pushed! Thank all involved in this development!!
For record, I attached the final patch I committed.


> Ok, it's fair enought for me.
> 
> I'll try to sort out my thought, then raise a follow-up discussion if necessary.

Thanks!

The followings are the open items and discussion points that I'm thinking of.

1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe.
 

2. Currently when the same foreign table is specified multiple times in the command, the extra information only for the
foreigntable found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not.
 

3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign table
isspecified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY to
theremote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not.
 

4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Fujii-san,

> >> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary. 
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.

Thank you for revising the v16 patch to v18 and pushing it.
Cool!

2021年4月8日(木) 22:14 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
>
>
> On 2021/04/08 22:02, Kohei KaiGai wrote:
> >> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary. 
>
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.
>
>
> > Ok, it's fair enought for me.
> >
> > I'll try to sort out my thought, then raise a follow-up discussion if necessary.
>
> Thanks!
>
> The followings are the open items and discussion points that I'm thinking of.
>
> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe. 
>
> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only for
theforeign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 
>
> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign table
isspecified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY to
theremote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not. 
>
> 4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.
>
> Regards,
>
> --
> Fujii Masao
> Advanced Computing Technology Center
> Research and Development Headquarters
> NTT DATA CORPORATION

On Thu, Apr 8, 2021 at 6:44 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> The followings are the open items and discussion points that I'm thinking of.
>
> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe. 

I think we should remove the unused enums/macros, instead we could
mention a note of the extensibility of those enums/macros in the
comments section around the enum/macro definitions.

> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only for
theforeign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 

IMO, the foreign truncate command should be constructed by collecting
all the information i.e. "TRUNCATE ft, ONLY ft" and let the remote
server execute how it wants to execute. That will be consistent and no
extra logic is required to track the already seen foreign tables while
foreign table collection/foreign truncate command is being prepared on
the local server.

I was thinking that the postgres throws error or warning for commands
such as truncate, vaccum, analyze when the same tables are specified,
but seems like that's not what it does.

> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign table
isspecified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY to
theremote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not. 

I'm okay with the behaviour as it is consistent with what ONLY does to
local tables. Documenting this behaviour(if not done already) is a
better way I think.

> 4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.

It will be good to have.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

2021年4月8日(木) 22:14 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/08 22:02, Kohei KaiGai wrote:
> >> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary. 
>
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.
>
>
> > Ok, it's fair enought for me.
> >
> > I'll try to sort out my thought, then raise a follow-up discussion if necessary.
>
> Thanks!
>
> The followings are the open items and discussion points that I'm thinking of.
>
> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe. 
>
> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only for
theforeign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 
>
> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign table
isspecified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY to
theremote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not. 
>
Prior to the discussion of 1-3, I like to clarify the role of foreign-tables.
(Likely, it will lead a natural conclusion for the above open items.)

As literal of SQL/MED (Management of External Data), a foreign table
is a representation of external data in PostgreSQL.
It allows to read and (optionally) write the external data wrapped by
FDW drivers, as if we usually read / write heap tables.
By the FDW-APIs, the core PostgreSQL does not care about the
structure, location, volume and other characteristics of
the external data itself. It expects FDW-APIs invocation will perform
as if we access a regular heap table.

On the other hands, we can say local tables are representation of
"internal" data in PostgreSQL.
A heap table is consists of one or more files (per BLCKSZ *
RELSEG_SIZE), and table-am intermediates
the on-disk data to/from on-memory structure (TupleTableSlot).
Here are no big differences in the concept. Ok?

As you know, ONLY clause controls whether TRUNCATE command shall run
on child-tables also, not only the parent.
If "ONLY parent_table" is given, its child tables are not picked up by
ExecuteTruncate(), unless child tables are not
listed up individually.
Then, once ExecuteTruncate() picked up the relations, it makes the
relations empty using table-am
(relation_set_new_filenode), and the callee
(heapam_relation_set_new_filenode) does not care about whether the
table is specified with ONLY, or not. It just makes the data
represented by the table empty (in transactional way).

So, how foreign tables shall perform?

Once ExecuteTruncate() picked up a foreign table, according to
ONLY-clause, does FDW driver shall consider
the context where the foreign tables are specified? And, what behavior
is consistent?
I think that FDW driver shall make the external data represented by
the foreign table empty, regardless of the
structure, location, volume and others.

Therefore, if we follow the above assumption, we don't need to inform
the context where foreign-tables are
picked up (TRUNCATE_REL_CONTEXT_*), so postgres_fdw shall not control
the remote TRUNCATE query
according to the flags. It always truncate the entire tables (if
multiple) on behalf of the foreign tables.

As an aside, if postgres_fdw maps are remote table with "ONLY" clause,
it is exactly a situation where we add
"ONLY" clause on the truncate command, because it is a representation
of the remote "ONLY parent_table" in
this case.

How about your thought?
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/09 11:05, Zhihong Yu wrote:
> 
> 
> On Thu, Apr 8, 2021 at 6:47 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com
<mailto:bharath.rupireddyforpostgres@gmail.com>>wrote:
 
> 
>     On Thu, Apr 8, 2021 at 6:44 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@oss.nttdata.com>>
wrote:
>      > The followings are the open items and discussion points that I'm thinking of.
>      >
>      > 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe.
 
> 
>     I think we should remove the unused enums/macros, instead we could
>     mention a note of the extensibility of those enums/macros in the
>     comments section around the enum/macro definitions.

+1


> 
>      > 2. Currently when the same foreign table is specified multiple times in the command, the extra information
onlyfor the foreign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not.
 
> 
>     IMO, the foreign truncate command should be constructed by collecting
>     all the information i.e. "TRUNCATE ft, ONLY ft" and let the remote
>     server execute how it wants to execute. That will be consistent and no
>     extra logic is required to track the already seen foreign tables while
>     foreign table collection/foreign truncate command is being prepared on
>     the local server.

But isn't it difficult for remote server to determine how to execute? Please imagine the case where there are four
tablesas follows.
 

- regular table "remote_parent" in the remote server
- regular table "remote_child" inheriting "remote_parent" table in the remote server
- foreign table "local_parent" in the local server, accessing "remote_parent" table
- regular table "local_child" inheriting "local_parent" table in the local server

When "TRUNCATE ONLY local_parent, local_parent" is executed, local_child is not truncated because of ONLY clause. Then
ifwe collect all the information about context, both TRUNCATE_REL_CONTEXT_NORMAL and _ONLY are passed to FDW. In this
casehow should FDW determine whether to use ONLY when issuing TRUNCATE command to the remote server? Isn't it difficult
todo that? If FDW determines not to use ONLY because _NORMAL flag is passed, both remote_parent and remote_child tables
aretruncated. That is, though both local_child and remote_child are the inheriting tables, isn't it strange that only
theformer is ignored and the latter is truncated?
 


> 
>     I was thinking that the postgres throws error or warning for commands
>     such as truncate, vaccum, analyze when the same tables are specified,
>     but seems like that's not what it does.
> 
>      > 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the
foreigntable is specified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table
withONLY to the remote server. Then only root table is truncated in remote server side, and the tables inheriting that
arenot truncated. Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not.
 
> 
>     I'm okay with the behaviour as it is consistent with what ONLY does to
>     local tables. Documenting this behaviour(if not done already) is a
>     better way I think.

+1


> 
>      > 4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.
> 
>     It will be good to have.

Patch attached.


> 
>     With Regards,
>     Bharath Rupireddy.
>     EnterpriseDB: http://www.enterprisedb.com <http://www.enterprisedb.com>
> 
> 
> w.r.t. point #1:
> bq. I think we should remove the unused enums/macros,
> 
> I agree. When there is more concrete use case which requires new enum, we can add enum whose meaning would be
clearer.

+1

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/09 12:33, Kohei KaiGai wrote:
> 2021年4月8日(木) 22:14 Fujii Masao <masao.fujii@oss.nttdata.com>:
>>
>> On 2021/04/08 22:02, Kohei KaiGai wrote:
>>>> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch. That
is,extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue the
discussionand change the behavior later if necessary.
 
>>
>> Pushed! Thank all involved in this development!!
>> For record, I attached the final patch I committed.
>>
>>
>>> Ok, it's fair enought for me.
>>>
>>> I'll try to sort out my thought, then raise a follow-up discussion if necessary.
>>
>> Thanks!
>>
>> The followings are the open items and discussion points that I'm thinking of.
>>
>> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe.
 
>>
>> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only for
theforeign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not.
 
>>
>> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign
tableis specified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY
tothe remote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not.
 
>>
> Prior to the discussion of 1-3, I like to clarify the role of foreign-tables.
> (Likely, it will lead a natural conclusion for the above open items.)
> 
> As literal of SQL/MED (Management of External Data), a foreign table
> is a representation of external data in PostgreSQL.
> It allows to read and (optionally) write the external data wrapped by
> FDW drivers, as if we usually read / write heap tables.
> By the FDW-APIs, the core PostgreSQL does not care about the
> structure, location, volume and other characteristics of
> the external data itself. It expects FDW-APIs invocation will perform
> as if we access a regular heap table.
> 
> On the other hands, we can say local tables are representation of
> "internal" data in PostgreSQL.
> A heap table is consists of one or more files (per BLCKSZ *
> RELSEG_SIZE), and table-am intermediates
> the on-disk data to/from on-memory structure (TupleTableSlot).
> Here are no big differences in the concept. Ok?
> 
> As you know, ONLY clause controls whether TRUNCATE command shall run
> on child-tables also, not only the parent.
> If "ONLY parent_table" is given, its child tables are not picked up by
> ExecuteTruncate(), unless child tables are not
> listed up individually.
> Then, once ExecuteTruncate() picked up the relations, it makes the
> relations empty using table-am
> (relation_set_new_filenode), and the callee
> (heapam_relation_set_new_filenode) does not care about whether the
> table is specified with ONLY, or not. It just makes the data
> represented by the table empty (in transactional way).
> 
> So, how foreign tables shall perform?
> 
> Once ExecuteTruncate() picked up a foreign table, according to
> ONLY-clause, does FDW driver shall consider
> the context where the foreign tables are specified? And, what behavior
> is consistent?
> I think that FDW driver shall make the external data represented by
> the foreign table empty, regardless of the
> structure, location, volume and others.
> 
> Therefore, if we follow the above assumption, we don't need to inform
> the context where foreign-tables are
> picked up (TRUNCATE_REL_CONTEXT_*), so postgres_fdw shall not control
> the remote TRUNCATE query
> according to the flags. It always truncate the entire tables (if
> multiple) on behalf of the foreign tables.

This makes me wonder if the information about CASCADE/RESTRICT (maybe also RESTART/CONTINUE) also should not be passed
toFDW. You're thinking that? Or only ONLY clause should be ignored for a foreign table?
 

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >      > 4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.
> >
> >     It will be good to have.
>
> Patch attached.

Tab completion patch LGTM and it works as expected.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >      > 2. Currently when the same foreign table is specified multiple times in the command, the extra information
onlyfor the foreign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 
> >
> >     IMO, the foreign truncate command should be constructed by collecting
> >     all the information i.e. "TRUNCATE ft, ONLY ft" and let the remote
> >     server execute how it wants to execute. That will be consistent and no
> >     extra logic is required to track the already seen foreign tables while
> >     foreign table collection/foreign truncate command is being prepared on
> >     the local server.
>
> But isn't it difficult for remote server to determine how to execute? Please imagine the case where there are four
tablesas follows. 
>
> - regular table "remote_parent" in the remote server
> - regular table "remote_child" inheriting "remote_parent" table in the remote server
> - foreign table "local_parent" in the local server, accessing "remote_parent" table
> - regular table "local_child" inheriting "local_parent" table in the local server
>
> When "TRUNCATE ONLY local_parent, local_parent" is executed, local_child is not truncated because of ONLY clause.
Thenif we collect all the information about context, both TRUNCATE_REL_CONTEXT_NORMAL and _ONLY are passed to FDW. In
thiscase how should FDW determine whether to use ONLY when issuing TRUNCATE command to the remote server? Isn't it
difficultto do that? If FDW determines not to use ONLY because _NORMAL flag is passed, both remote_parent and
remote_childtables are truncated. That is, though both local_child and remote_child are the inheriting tables, isn't it
strangethat only the former is ignored and the latter is truncated? 

My understanding was wrong. I see below code from ExecuteTruncate:
        /* don't throw error for "TRUNCATE foo, foo" */
        if (list_member_oid(relids, myrelid))
        {
            table_close(rel, lockmode);
            continue;
        }

This basically tells us that the first occurence of a table is
considered, rest all ignored. This is what we are going to have in our
relids_extra and relids. So, we will be sending only the first
occurence info to the foreign truncate command.  I agree with the
current approach "i.e., collect the extra info about table found first
if the same table is specified multiple times" for the same reason
that "local tables are also treated the same way."

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

2021年4月9日(金) 22:51 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/09 12:33, Kohei KaiGai wrote:
> > 2021年4月8日(木) 22:14 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >>
> >> On 2021/04/08 22:02, Kohei KaiGai wrote:
> >>>> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch.
Thatis, extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue
thediscussion and change the behavior later if necessary. 
> >>
> >> Pushed! Thank all involved in this development!!
> >> For record, I attached the final patch I committed.
> >>
> >>
> >>> Ok, it's fair enought for me.
> >>>
> >>> I'll try to sort out my thought, then raise a follow-up discussion if necessary.
> >>
> >> Thanks!
> >>
> >> The followings are the open items and discussion points that I'm thinking of.
> >>
> >> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe. 
> >>
> >> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only
forthe foreign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 
> >>
> >> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign
tableis specified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY
tothe remote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not. 
> >>
> > Prior to the discussion of 1-3, I like to clarify the role of foreign-tables.
> > (Likely, it will lead a natural conclusion for the above open items.)
> >
> > As literal of SQL/MED (Management of External Data), a foreign table
> > is a representation of external data in PostgreSQL.
> > It allows to read and (optionally) write the external data wrapped by
> > FDW drivers, as if we usually read / write heap tables.
> > By the FDW-APIs, the core PostgreSQL does not care about the
> > structure, location, volume and other characteristics of
> > the external data itself. It expects FDW-APIs invocation will perform
> > as if we access a regular heap table.
> >
> > On the other hands, we can say local tables are representation of
> > "internal" data in PostgreSQL.
> > A heap table is consists of one or more files (per BLCKSZ *
> > RELSEG_SIZE), and table-am intermediates
> > the on-disk data to/from on-memory structure (TupleTableSlot).
> > Here are no big differences in the concept. Ok?
> >
> > As you know, ONLY clause controls whether TRUNCATE command shall run
> > on child-tables also, not only the parent.
> > If "ONLY parent_table" is given, its child tables are not picked up by
> > ExecuteTruncate(), unless child tables are not
> > listed up individually.
> > Then, once ExecuteTruncate() picked up the relations, it makes the
> > relations empty using table-am
> > (relation_set_new_filenode), and the callee
> > (heapam_relation_set_new_filenode) does not care about whether the
> > table is specified with ONLY, or not. It just makes the data
> > represented by the table empty (in transactional way).
> >
> > So, how foreign tables shall perform?
> >
> > Once ExecuteTruncate() picked up a foreign table, according to
> > ONLY-clause, does FDW driver shall consider
> > the context where the foreign tables are specified? And, what behavior
> > is consistent?
> > I think that FDW driver shall make the external data represented by
> > the foreign table empty, regardless of the
> > structure, location, volume and others.
> >
> > Therefore, if we follow the above assumption, we don't need to inform
> > the context where foreign-tables are
> > picked up (TRUNCATE_REL_CONTEXT_*), so postgres_fdw shall not control
> > the remote TRUNCATE query
> > according to the flags. It always truncate the entire tables (if
> > multiple) on behalf of the foreign tables.
>
> This makes me wonder if the information about CASCADE/RESTRICT (maybe also RESTART/CONTINUE) also should not be
passedto FDW. You're thinking that? Or only ONLY clause should be ignored for a foreign table? 
>
I think the above information (DropBehavior and restart_seqs) are
valuable to pass.

The CASCADE/RESTRICT clause controls whether the truncate command also
eliminates
the rows that blocks to delete (FKs in RDBMS). Only FDW driver can
know whether the
external data has "removal-blocker", thus we need to pass the
DropBehavior for the callback.

The RESTART/CONTINUE clause also controle whether the truncate command restart
the relevant resources that is associated with the target table
(Sequences in RDBMS).
Only FDW driver can know whether the external data has relevant
resources to reset,
thus we need to pass the "restart_seqs" for the callback.

Unlike above two parameters, the role of ONLY-clause is already
finished at the time
when ExecuteTruncate() picked up the target relations, from the
standpoint of above
understanding of foreign-tables and external data.

Thought?

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Find attached language fixes.

Thanks for the patches.

> I'm also proposing to convert an if/else to an switch(), since I don't like
> "if/else if" without an "else", and since the compiler can warn about missing
> enum values in switch cases.

I think we have a good bunch of if, else-if (without else) in the code
base, and I'm not sure the compilers have warned about them. Apart
from that whether if-else or switch-case is just a coding choice. And
we have only two values for DropBehavior enum i.e DROP_RESTRICT and
DROP_CASCADE(I'm not sure we will extend this enum to have more
values), if we have more then switch case would have looked cleaner.
But IMO, existing if and else-if would be good. Having said that, it's
up to the committer which one they think better in this case.

> You could also write:
> | Assert(behavior == DROP_RESTRICT || behavior == DROP_CASCADE)

IMO, we don't need to assert on behaviour as we just carry that
variable from ExecuteTruncateGuts to postgresExecForeignTruncate
without any modifications. And ExecuteTruncateGuts would get it from
the syntaxer, so no point it will have a different value than
DROP_RESTRICT or DROP_CASCADE.

> Also, you currently test:
> > +             if (extra & TRUNCATE_REL_CONTEXT_ONLY)
>
> but TRUNCATE_REL_ aren't indepedent bits, so shouldn't be tested with "&".

Yeah this is an issue. We could just change the #defines to values
0x0001, 0x0002, 0x0004, 0x0008 ... 0x0020 and so on and then testing
with & would work. So, this way, more than option can be multiplexed
into the same int value. To multiplex, we need to think: will there be
a scenario where a single rel in the truncate can have multiple
options at a time and do we want to distinguish these options while
deparsing?

#define TRUNCATE_REL_CONTEXT_NORMAL      0x0001 /* specified without
ONLY clause */
#define TRUNCATE_REL_CONTEXT_ONLY         0x0002 /* specified with
ONLY clause */
#define TRUNCATE_REL_CONTEXT_CASCADING    0x0004   /* not specified
but truncated

And I'm not sure what's the agreement on retaining or removing #define
values, currently I see only TRUNCATE_REL_CONTEXT_ONLY is being used,
others are just being set but not used. As I said upthread, it will be
good to remove the unused macros/enums, retain only the ones that are
used, especially TRUNCATE_REL_CONTEXT_CASCADING this is not required I
feel, because we can add the child partitions that are foreign tables
to relids as just normal foreign tables with TRUNCATE_REL_CONTEXT_ONLY
option.

On the patches:
0001-WIP-doc-review-Allow-TRUNCATE-command-to-truncate-fo.patch ---> LGTM.
0002-Convert-an-if-else-if-without-an-else-to-a-switch.patch. --> IMO,
we can ignore this patch.
0003-Test-integer-using-and-not.patch --> if we redefine the marcos to
multiplex them into a single int value, we don't need this patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/11 19:15, Bharath Rupireddy wrote:
> On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>> Find attached language fixes.
> 
> Thanks for the patches.

Thanks for the patches!

0001 patch basically looks good to me.

+     <literal>behavior</literal> must be specified as
+     <literal>DROP_RESTRICT</literal> or <literal>DROP_CASCADE</literal>.
+     If specified as <literal>DROP_RESTRICT</literal>, the
+     <literal>RESTRICT</literal> option will be included in the
       <command>TRUNCATE</command> command.
+     If specified as <literal>DROP_CASCADE</literal>, the
+     <literal>CASCADE</literal> option will be included.

Maybe "will be included" is confusing? Because FDW might not include
the RESTRICT in the TRUNCATE command that it will issue
when DROP_RESTRICT is specified, for example. To be more precise,
we should document something like "If specified as DROP_RESTRICT,
the RESTRICT option was included in the original TRUNCATE command"?


>> I'm also proposing to convert an if/else to an switch(), since I don't like
>> "if/else if" without an "else", and since the compiler can warn about missing
>> enum values in switch cases.
> 
> I think we have a good bunch of if, else-if (without else) in the code
> base, and I'm not sure the compilers have warned about them. Apart
> from that whether if-else or switch-case is just a coding choice. And
> we have only two values for DropBehavior enum i.e DROP_RESTRICT and
> DROP_CASCADE(I'm not sure we will extend this enum to have more
> values), if we have more then switch case would have looked cleaner.
> But IMO, existing if and else-if would be good. Having said that, it's
> up to the committer which one they think better in this case.

Either works at least for me. Also for now I have no strong opinion
to change the condition so that it uses switch().


>> You could also write:
>> | Assert(behavior == DROP_RESTRICT || behavior == DROP_CASCADE)
> 
> IMO, we don't need to assert on behaviour as we just carry that
> variable from ExecuteTruncateGuts to postgresExecForeignTruncate
> without any modifications. And ExecuteTruncateGuts would get it from
> the syntaxer, so no point it will have a different value than
> DROP_RESTRICT or DROP_CASCADE.
> 
>> Also, you currently test:
>>> +             if (extra & TRUNCATE_REL_CONTEXT_ONLY)
>>
>> but TRUNCATE_REL_ aren't indepedent bits, so shouldn't be tested with "&".

You're right.


> Yeah this is an issue. We could just change the #defines to values
> 0x0001, 0x0002, 0x0004, 0x0008 ... 0x0020 and so on and then testing
> with & would work. So, this way, more than option can be multiplexed
> into the same int value. To multiplex, we need to think: will there be
> a scenario where a single rel in the truncate can have multiple
> options at a time and do we want to distinguish these options while
> deparsing?
> 
> #define TRUNCATE_REL_CONTEXT_NORMAL      0x0001 /* specified without
> ONLY clause */
> #define TRUNCATE_REL_CONTEXT_ONLY         0x0002 /* specified with
> ONLY clause */
> #define TRUNCATE_REL_CONTEXT_CASCADING    0x0004   /* not specified
> but truncated
> 
> And I'm not sure what's the agreement on retaining or removing #define
> values, currently I see only TRUNCATE_REL_CONTEXT_ONLY is being used,
> others are just being set but not used. As I said upthread, it will be
> good to remove the unused macros/enums, retain only the ones that are
> used, especially TRUNCATE_REL_CONTEXT_CASCADING this is not required I
> feel, because we can add the child partitions that are foreign tables
> to relids as just normal foreign tables with TRUNCATE_REL_CONTEXT_ONLY
> option.

Our current consensus seems that TRUNCATE_REL_CONTEXT_NORMAL and
TRUNCATE_REL_CONTEXT_CASCADING should be removed because they are not used.
Since Kaigai-san thinks to remove the extra information at all,
I guess he also agrees to remove those both TRUNCATE_REL_CONTEXT_NORMAL
and _CASCADING. If this is right, we should apply 0003 patch and remove
those two macro values. Or we should make the extra info boolean value
instead of int, i.e., it indicates whether ONLY was specified or not.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/09 23:10, Bharath Rupireddy wrote:
> On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>       > 4. Tab-completion for TRUNCATE should be updated so that also foreign tables are displayed.
>>>
>>>      It will be good to have.
>>
>> Patch attached.
> 
> Tab completion patch LGTM and it works as expected.

Thanks for the review! Pushed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Apr 13, 2021 at 6:27 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Sun, Apr 11, 2021 at 03:45:36PM +0530, Bharath Rupireddy wrote:
> > On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > > Also, you currently test:
> > > > +             if (extra & TRUNCATE_REL_CONTEXT_ONLY)
> > >
> > > but TRUNCATE_REL_ aren't indepedent bits, so shouldn't be tested with "&".
> >
> > Yeah this is an issue. We could just change the #defines to values
> > 0x0001, 0x0002, 0x0004, 0x0008 ... 0x0020 and so on and then testing
> > with & would work. So, this way, more than option can be multiplexed
> > into the same int value. To multiplex, we need to think: will there be
> > a scenario where a single rel in the truncate can have multiple
> > options at a time and do we want to distinguish these options while
> > deparsing?
> >
> > #define TRUNCATE_REL_CONTEXT_NORMAL      0x0001 /* specified without
> > ONLY clause */
> > #define TRUNCATE_REL_CONTEXT_ONLY         0x0002 /* specified with
> > ONLY clause */
> > #define TRUNCATE_REL_CONTEXT_CASCADING    0x0004   /* not specified
> > but truncated
> >
> > And I'm not sure what's the agreement on retaining or removing #define
> > values, currently I see only TRUNCATE_REL_CONTEXT_ONLY is being used,
> > others are just being set but not used. As I said upthread, it will be
> > good to remove the unused macros/enums, retain only the ones that are
> > used, especially TRUNCATE_REL_CONTEXT_CASCADING this is not required I
> > feel, because we can add the child partitions that are foreign tables
> > to relids as just normal foreign tables with TRUNCATE_REL_CONTEXT_ONLY
> > option.
>
> Converting to "bits" would collapse TRUNCATE_REL_CONTEXT_ONLY and
> TRUNCATE_REL_CONTEXT_NORMAL into a single bit.  TRUNCATE_REL_CONTEXT_CASCADING
> could optionally be removed.
>
> +1 to convert to bits instead of changing "&" to "==".

Thanks for the patches.

I agree to convert to bits and pass it as int value which is
extensible i.e. we can pass more extra parameters across if required.
Also I'm not in favour of removing relids_extra altogether, we might
need this to send some info in future.

Both 0001 and 0002(along with the new phrasings) look good to me.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/13 10:21, Bharath Rupireddy wrote:
> I agree to convert to bits and pass it as int value which is
> extensible i.e. we can pass more extra parameters across if required.

Looks good to me.


> Also I'm not in favour of removing relids_extra altogether, we might
> need this to send some info in future.
> 
> Both 0001 and 0002(along with the new phrasings) look good to me.

Thanks for updating the patches!

One question is; "CONTEXT" of "TRUNCATE_REL_CONTEXT_ONLY" is required?
If not, I'm tempted to shorten the name to "TRUNCATE_REL_ONLY" or something.

+     <structname>Relation</structname> data structures for each
+     foreign tables to be truncated.

"foreign tables" should be "foreign table" because it follows "each"?

+    <para>
+     <literal>behavior</literal> is either
+     <literal>DROP_RESTRICT</literal> or <literal>DROP_CASCADE</literal>.
+     <literal>DROP_CASCADE</literal> indicates that the
+     <literal>CASCADE</literal> option was specified in the original
       <command>TRUNCATE</command> command.

Why did you remove the description for DROP_RESTRICT?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

2021年4月9日(金) 23:49 Kohei KaiGai <kaigai@heterodb.com>:
>
> 2021年4月9日(金) 22:51 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >
> > On 2021/04/09 12:33, Kohei KaiGai wrote:
> > > 2021年4月8日(木) 22:14 Fujii Masao <masao.fujii@oss.nttdata.com>:
> > >>
> > >> On 2021/04/08 22:02, Kohei KaiGai wrote:
> > >>>> Anyway, attached is the updated version of the patch. This is still based on the latest Kazutaka-san's patch.
Thatis, extra list for ONLY is still passed to FDW. What about committing this version at first? Then we can continue
thediscussion and change the behavior later if necessary. 
> > >>
> > >> Pushed! Thank all involved in this development!!
> > >> For record, I attached the final patch I committed.
> > >>
> > >>
> > >>> Ok, it's fair enought for me.
> > >>>
> > >>> I'll try to sort out my thought, then raise a follow-up discussion if necessary.
> > >>
> > >> Thanks!
> > >>
> > >> The followings are the open items and discussion points that I'm thinking of.
> > >>
> > >> 1. Currently the extra information (TRUNCATE_REL_CONTEXT_NORMAL, TRUNCATE_REL_CONTEXT_ONLY or
TRUNCATE_REL_CONTEXT_CASCADING)about how a foreign table was specified as the target to truncate in TRUNCATE command is
collectedand passed to FDW. Does this really need to be passed to FDW? Seems Stephen, Michael and I think that's
necessary.But Kaigai-san does not. I also think that TRUNCATE_REL_CONTEXT_CASCADING can be removed because there seems
nouse case for that maybe. 
> > >>
> > >> 2. Currently when the same foreign table is specified multiple times in the command, the extra information only
forthe foreign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed,
TRUNCATE_REL_CONTEXT_NORMALis collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should
collectall, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e.,
collectthe extra info about table found first if the same table is specified multiple times) is good because even local
tablesare also treated the same way. But Kaigai-san does not. 
> > >>
> > >> 3. Currently postgres_fdw specifies ONLY clause in TRUNCATE command that it constructs. That is, if the foreign
tableis specified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY
tothe remote server. Then only root table is truncated in remote server side, and the tables inheriting that are not
truncated.Is this behavior desirable? Seems Michael and I think this behavior is OK. But Kaigai-san does not. 
> > >>
> > > Prior to the discussion of 1-3, I like to clarify the role of foreign-tables.
> > > (Likely, it will lead a natural conclusion for the above open items.)
> > >
> > > As literal of SQL/MED (Management of External Data), a foreign table
> > > is a representation of external data in PostgreSQL.
> > > It allows to read and (optionally) write the external data wrapped by
> > > FDW drivers, as if we usually read / write heap tables.
> > > By the FDW-APIs, the core PostgreSQL does not care about the
> > > structure, location, volume and other characteristics of
> > > the external data itself. It expects FDW-APIs invocation will perform
> > > as if we access a regular heap table.
> > >
> > > On the other hands, we can say local tables are representation of
> > > "internal" data in PostgreSQL.
> > > A heap table is consists of one or more files (per BLCKSZ *
> > > RELSEG_SIZE), and table-am intermediates
> > > the on-disk data to/from on-memory structure (TupleTableSlot).
> > > Here are no big differences in the concept. Ok?
> > >
> > > As you know, ONLY clause controls whether TRUNCATE command shall run
> > > on child-tables also, not only the parent.
> > > If "ONLY parent_table" is given, its child tables are not picked up by
> > > ExecuteTruncate(), unless child tables are not
> > > listed up individually.
> > > Then, once ExecuteTruncate() picked up the relations, it makes the
> > > relations empty using table-am
> > > (relation_set_new_filenode), and the callee
> > > (heapam_relation_set_new_filenode) does not care about whether the
> > > table is specified with ONLY, or not. It just makes the data
> > > represented by the table empty (in transactional way).
> > >
> > > So, how foreign tables shall perform?
> > >
> > > Once ExecuteTruncate() picked up a foreign table, according to
> > > ONLY-clause, does FDW driver shall consider
> > > the context where the foreign tables are specified? And, what behavior
> > > is consistent?
> > > I think that FDW driver shall make the external data represented by
> > > the foreign table empty, regardless of the
> > > structure, location, volume and others.
> > >
> > > Therefore, if we follow the above assumption, we don't need to inform
> > > the context where foreign-tables are
> > > picked up (TRUNCATE_REL_CONTEXT_*), so postgres_fdw shall not control
> > > the remote TRUNCATE query
> > > according to the flags. It always truncate the entire tables (if
> > > multiple) on behalf of the foreign tables.
> >
> > This makes me wonder if the information about CASCADE/RESTRICT (maybe also RESTART/CONTINUE) also should not be
passedto FDW. You're thinking that? Or only ONLY clause should be ignored for a foreign table? 
> >
> I think the above information (DropBehavior and restart_seqs) are
> valuable to pass.
>
> The CASCADE/RESTRICT clause controls whether the truncate command also
> eliminates
> the rows that blocks to delete (FKs in RDBMS). Only FDW driver can
> know whether the
> external data has "removal-blocker", thus we need to pass the
> DropBehavior for the callback.
>
> The RESTART/CONTINUE clause also controle whether the truncate command restart
> the relevant resources that is associated with the target table
> (Sequences in RDBMS).
> Only FDW driver can know whether the external data has relevant
> resources to reset,
> thus we need to pass the "restart_seqs" for the callback.
>
> Unlike above two parameters, the role of ONLY-clause is already
> finished at the time
> when ExecuteTruncate() picked up the target relations, from the
> standpoint of above
> understanding of foreign-tables and external data.
>
> Thought?
>
Let me remind the discussion at the design level.

If postgres_fdw (and other FDW drivers) needs to consider whether
ONLY-clause is given
on the foreign tables of them, what does a foreign table represent in
PostgreSQL system?

My assumption is, a foreign table provides a view to external data, as
if it performs like a table.
TRUNCATE command eliminates all the segment files, even if a table
contains multiple
underlying files, never eliminate them partially.
If a foreign table is equivalent to a table in SQL operation level,
indeed, ONLY-clause controls
which tables are picked up by the TRUNCATE command, but never controls
which portion of
the data shall be eliminated. So, I conclude that
ExecForeignTruncate() shall eliminate the entire
external data on behalf of a foreign table, regardless of ONLY-clause.

I think it is more significant to clarify prior to the implementation details.
How about your opinions?

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/13 12:46, Justin Pryzby wrote:
> On Tue, Apr 13, 2021 at 12:38:35PM +0900, Fujii Masao wrote:
>> +     <structname>Relation</structname> data structures for each
>> +     foreign tables to be truncated.
>>
>> "foreign tables" should be "foreign table" because it follows "each"?
> 
> Yes, you're right.
> 
>> +    <para>
>> +     <literal>behavior</literal> is either
>> +     <literal>DROP_RESTRICT</literal> or <literal>DROP_CASCADE</literal>.
>> +     <literal>DROP_CASCADE</literal> indicates that the
>> +     <literal>CASCADE</literal> option was specified in the original
>>        <command>TRUNCATE</command> command.
>>
>> Why did you remove the description for DROP_RESTRICT?
> 
> Because in order to handle the default/unspecified case, the description was
> going to need to be something like:
> 
> | DROP_RESTRICT indicates that the RESTRICT option was specified in the original
> | truncate command (or CASCADE option was NOT specified).

What about using "requested" instead of "specified"? If neither RESTRICT nor
CASCADE is specified, we can think that user requested the default behavior,
i.e., RESTRICT. So, for example,

     <literal>behavior</literal> is either <literal>DROP_RESTRICT</literal> or
     <literal>DROP_CASCADE</literal>, which indicates that the
     <literal>RESTRICT</literal> or <literal>CASCADE</literal> option was
     requested in the original <command>TRUNCATE</command> command, respectively.

Regards,


-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/13 14:22, Kohei KaiGai wrote:
> Let me remind the discussion at the design level.
> 
> If postgres_fdw (and other FDW drivers) needs to consider whether
> ONLY-clause is given
> on the foreign tables of them, what does a foreign table represent in
> PostgreSQL system?
> 
> My assumption is, a foreign table provides a view to external data, as
> if it performs like a table.
> TRUNCATE command eliminates all the segment files, even if a table
> contains multiple
> underlying files, never eliminate them partially.
> If a foreign table is equivalent to a table in SQL operation level,
> indeed, ONLY-clause controls
> which tables are picked up by the TRUNCATE command, but never controls
> which portion of
> the data shall be eliminated. So, I conclude that
> ExecForeignTruncate() shall eliminate the entire
> external data on behalf of a foreign table, regardless of ONLY-clause.
> 
> I think it is more significant to clarify prior to the implementation details.
> How about your opinions?

I'm still thinking that it's better to pass all information including
ONLY clause about TRUNCATE command to FDW and leave FDW to determine
how to use them. How postgres_fdw should use the information about ONLY
is debetable. But for now IMO that users who explicitly specify ONLY clause for
foreign tables understand the structure of remote tables and want to use ONLY
in TRUNCATE command issued by postgres_fdw. But my opinion might be minority,
so I'd like to hear more opinion about this, from other developers.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Tue, 13 Apr 2021 16:17:12 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> 
> 
> On 2021/04/13 14:22, Kohei KaiGai wrote:
> > Let me remind the discussion at the design level.
> > If postgres_fdw (and other FDW drivers) needs to consider whether
> > ONLY-clause is given
> > on the foreign tables of them, what does a foreign table represent in
> > PostgreSQL system?
> > My assumption is, a foreign table provides a view to external data, as
> > if it performs like a table.
> > TRUNCATE command eliminates all the segment files, even if a table
> > contains multiple
> > underlying files, never eliminate them partially.
> > If a foreign table is equivalent to a table in SQL operation level,
> > indeed, ONLY-clause controls
> > which tables are picked up by the TRUNCATE command, but never controls
> > which portion of
> > the data shall be eliminated. So, I conclude that
> > ExecForeignTruncate() shall eliminate the entire
> > external data on behalf of a foreign table, regardless of ONLY-clause.
> > I think it is more significant to clarify prior to the implementation
> > details.
> > How about your opinions?
> 
> I'm still thinking that it's better to pass all information including
> ONLY clause about TRUNCATE command to FDW and leave FDW to determine
> how to use them. How postgres_fdw should use the information about
> ONLY
> is debetable. But for now IMO that users who explicitly specify ONLY
> clause for
> foreign tables understand the structure of remote tables and want to
> use ONLY
> in TRUNCATE command issued by postgres_fdw. But my opinion might be
> minority,
> so I'd like to hear more opinion about this, from other developers.

From the syntactical point of view, my opinion on this is that the
"ONLY" in "TRUNCATE ONLY" is assumed to be consumed to tell to
disregard local children so it cannot be propagate further whichever
the target relation has children or not.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

2021年4月13日(火) 16:17 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/13 14:22, Kohei KaiGai wrote:
> > Let me remind the discussion at the design level.
> >
> > If postgres_fdw (and other FDW drivers) needs to consider whether
> > ONLY-clause is given
> > on the foreign tables of them, what does a foreign table represent in
> > PostgreSQL system?
> >
> > My assumption is, a foreign table provides a view to external data, as
> > if it performs like a table.
> > TRUNCATE command eliminates all the segment files, even if a table
> > contains multiple
> > underlying files, never eliminate them partially.
> > If a foreign table is equivalent to a table in SQL operation level,
> > indeed, ONLY-clause controls
> > which tables are picked up by the TRUNCATE command, but never controls
> > which portion of
> > the data shall be eliminated. So, I conclude that
> > ExecForeignTruncate() shall eliminate the entire
> > external data on behalf of a foreign table, regardless of ONLY-clause.
> >
> > I think it is more significant to clarify prior to the implementation details.
> > How about your opinions?
>
> I'm still thinking that it's better to pass all information including
> ONLY clause about TRUNCATE command to FDW and leave FDW to determine
> how to use them. How postgres_fdw should use the information about ONLY
> is debetable. But for now IMO that users who explicitly specify ONLY clause for
> foreign tables understand the structure of remote tables and want to use ONLY
> in TRUNCATE command issued by postgres_fdw. But my opinion might be minority,
> so I'd like to hear more opinion about this, from other developers.
>
Here are two points to discuss.

Regarding to the FDW-APIs, yes, nobody can deny someone want to implement
their own FDW module that adds special handling when its foreign table
is specified
with ONLY-clause, even if we usually ignore.


On the other hand, when we consider a foreign table is an abstraction
of an external
data source, at least, the current postgres_fdw's behavior is not consistent.

When a foreign table by postgres_fdw that maps a remote parent table,
has a local
child table,

This command shows all the rows from both of local and remote.

postgres=# select * from f_table ;
 id |              v
----+-----------------------------
  1 | remote table t_parent id=1
  2 | remote table t_parent id=2
  3 | remote table t_parent id=3
 10 | remote table t_child1 id=10
 11 | remote table t_child1 id=11
 12 | remote table t_child1 id=12
 20 | remote table t_child2 id=20
 21 | remote table t_child2 id=21
 22 | remote table t_child2 id=22
 50 | it is l_child id=50
 51 | it is l_child id=51
 52 | it is l_child id=52
 53 | it is l_child id=53
(13 rows)

If f_table is specified with "ONLY", it picks up only the parent table
(f_table),
however, ONLY-clause is not push down to the remote side.

postgres=# select * from only f_table ;
 id |              v
----+-----------------------------
  1 | remote table t_parent id=1
  2 | remote table t_parent id=2
  3 | remote table t_parent id=3
 10 | remote table t_child1 id=10
 11 | remote table t_child1 id=11
 12 | remote table t_child1 id=12
 20 | remote table t_child2 id=20
 21 | remote table t_child2 id=21
 22 | remote table t_child2 id=22
(9 rows)

On the other hands, TRUNCATE ONLY f_table works as follows...

postgres=# truncate only f_table;
TRUNCATE TABLE
postgres=# select * from f_table ;
 id |              v
----+-----------------------------
 10 | remote table t_child1 id=10
 11 | remote table t_child1 id=11
 12 | remote table t_child1 id=12
 20 | remote table t_child2 id=20
 21 | remote table t_child2 id=21
 22 | remote table t_child2 id=22
 50 | it is l_child id=50
 51 | it is l_child id=51
 52 | it is l_child id=52
 53 | it is l_child id=53
(10 rows)

It eliminates the rows only from the remote parent table although it
is a part of the foreign table.

My expectation at the above command shows rows from the local child
table (id=50...53).

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On Tue, Apr 13, 2021 at 2:37 PM Kohei KaiGai <kaigai@heterodb.com> wrote:
> Here are two points to discuss.
>
> Regarding to the FDW-APIs, yes, nobody can deny someone want to implement
> their own FDW module that adds special handling when its foreign table
> is specified
> with ONLY-clause, even if we usually ignore.
>
>
> On the other hand, when we consider a foreign table is an abstraction
> of an external
> data source, at least, the current postgres_fdw's behavior is not consistent.
>
> When a foreign table by postgres_fdw that maps a remote parent table,
> has a local
> child table,
>
> This command shows all the rows from both of local and remote.
>
> postgres=# select * from f_table ;
>  id |              v
> ----+-----------------------------
>   1 | remote table t_parent id=1
>   2 | remote table t_parent id=2
>   3 | remote table t_parent id=3
>  10 | remote table t_child1 id=10
>  11 | remote table t_child1 id=11
>  12 | remote table t_child1 id=12
>  20 | remote table t_child2 id=20
>  21 | remote table t_child2 id=21
>  22 | remote table t_child2 id=22
>  50 | it is l_child id=50
>  51 | it is l_child id=51
>  52 | it is l_child id=52
>  53 | it is l_child id=53
> (13 rows)
>
> If f_table is specified with "ONLY", it picks up only the parent table
> (f_table),
> however, ONLY-clause is not push down to the remote side.
>
> postgres=# select * from only f_table ;
>  id |              v
> ----+-----------------------------
>   1 | remote table t_parent id=1
>   2 | remote table t_parent id=2
>   3 | remote table t_parent id=3
>  10 | remote table t_child1 id=10
>  11 | remote table t_child1 id=11
>  12 | remote table t_child1 id=12
>  20 | remote table t_child2 id=20
>  21 | remote table t_child2 id=21
>  22 | remote table t_child2 id=22
> (9 rows)
>
> On the other hands, TRUNCATE ONLY f_table works as follows...
>
> postgres=# truncate only f_table;
> TRUNCATE TABLE
> postgres=# select * from f_table ;
>  id |              v
> ----+-----------------------------
>  10 | remote table t_child1 id=10
>  11 | remote table t_child1 id=11
>  12 | remote table t_child1 id=12
>  20 | remote table t_child2 id=20
>  21 | remote table t_child2 id=21
>  22 | remote table t_child2 id=22
>  50 | it is l_child id=50
>  51 | it is l_child id=51
>  52 | it is l_child id=52
>  53 | it is l_child id=53
> (10 rows)
>
> It eliminates the rows only from the remote parent table although it
> is a part of the foreign table.
>
> My expectation at the above command shows rows from the local child
> table (id=50...53).

Yeah, ONLY clause is not pushed to the remote server in case of SELECT
commands. This is also true for DELETE and UPDATE commands on foreign
tables. I'm not sure if it wasn't thought necessary or if there is an
issue to push it or I may be missing something here. I think we can
start a separate thread to see other hackers' opinions on this.

I'm not sure whether all the clauses that are possible for
SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
server by postgres_fdw.

Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
If we were to keep it consistent across all foreign commands that
ONLY clause is not pushed to remote server, then we can restrict for
TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
don't see any real problem in pushing the ONLY clause, at least in
case of TRUNCATE.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

2021年4月13日(火) 21:03 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>
> On Tue, Apr 13, 2021 at 2:37 PM Kohei KaiGai <kaigai@heterodb.com> wrote:
> > Here are two points to discuss.
> >
> > Regarding to the FDW-APIs, yes, nobody can deny someone want to implement
> > their own FDW module that adds special handling when its foreign table
> > is specified
> > with ONLY-clause, even if we usually ignore.
> >
> >
> > On the other hand, when we consider a foreign table is an abstraction
> > of an external
> > data source, at least, the current postgres_fdw's behavior is not consistent.
> >
> > When a foreign table by postgres_fdw that maps a remote parent table,
> > has a local
> > child table,
> >
> > This command shows all the rows from both of local and remote.
> >
> > postgres=# select * from f_table ;
> >  id |              v
> > ----+-----------------------------
> >   1 | remote table t_parent id=1
> >   2 | remote table t_parent id=2
> >   3 | remote table t_parent id=3
> >  10 | remote table t_child1 id=10
> >  11 | remote table t_child1 id=11
> >  12 | remote table t_child1 id=12
> >  20 | remote table t_child2 id=20
> >  21 | remote table t_child2 id=21
> >  22 | remote table t_child2 id=22
> >  50 | it is l_child id=50
> >  51 | it is l_child id=51
> >  52 | it is l_child id=52
> >  53 | it is l_child id=53
> > (13 rows)
> >
> > If f_table is specified with "ONLY", it picks up only the parent table
> > (f_table),
> > however, ONLY-clause is not push down to the remote side.
> >
> > postgres=# select * from only f_table ;
> >  id |              v
> > ----+-----------------------------
> >   1 | remote table t_parent id=1
> >   2 | remote table t_parent id=2
> >   3 | remote table t_parent id=3
> >  10 | remote table t_child1 id=10
> >  11 | remote table t_child1 id=11
> >  12 | remote table t_child1 id=12
> >  20 | remote table t_child2 id=20
> >  21 | remote table t_child2 id=21
> >  22 | remote table t_child2 id=22
> > (9 rows)
> >
> > On the other hands, TRUNCATE ONLY f_table works as follows...
> >
> > postgres=# truncate only f_table;
> > TRUNCATE TABLE
> > postgres=# select * from f_table ;
> >  id |              v
> > ----+-----------------------------
> >  10 | remote table t_child1 id=10
> >  11 | remote table t_child1 id=11
> >  12 | remote table t_child1 id=12
> >  20 | remote table t_child2 id=20
> >  21 | remote table t_child2 id=21
> >  22 | remote table t_child2 id=22
> >  50 | it is l_child id=50
> >  51 | it is l_child id=51
> >  52 | it is l_child id=52
> >  53 | it is l_child id=53
> > (10 rows)
> >
> > It eliminates the rows only from the remote parent table although it
> > is a part of the foreign table.
> >
> > My expectation at the above command shows rows from the local child
> > table (id=50...53).
>
> Yeah, ONLY clause is not pushed to the remote server in case of SELECT
> commands. This is also true for DELETE and UPDATE commands on foreign
> tables. I'm not sure if it wasn't thought necessary or if there is an
> issue to push it or I may be missing something here. I think we can
> start a separate thread to see other hackers' opinions on this.
>
> I'm not sure whether all the clauses that are possible for
> SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
> server by postgres_fdw.
>
> Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
> which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
> If we were to keep it consistent across all foreign commands that
> ONLY clause is not pushed to remote server, then we can restrict for
> TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
> just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
> don't see any real problem in pushing the ONLY clause, at least in
> case of TRUNCATE.
>
If ONLY-clause would be pushed down to the remote query of postgres_fdw,
what does the foreign-table represent in the local system?

In my understanding, a local foreign table by postgres_fdw is a
representation of
entire tree of the remote parent table and its children.
Thus, we have assumed that DML command fetches rows from the remote
parent table without ONLY-clause, once PostgreSQL picked up the foreign table
as a scan target.
I think we don't need to adjust definitions of the role of
foreign-table, even if
it represents non-RDBMS data sources.

If a foreign table by postgres_fdw supports a special table option to
indicate adding
ONLY-clause when remote query uses remote tables, it is suitable to
add ONLY-clause
on the remote TRUNCATE command also, not only SELECT/INSERT/UPDATE/DELETE.
In the other words, if a foreign-table represents only a remote parent
table, it is
suitable to truncate only the remote parent table.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

On 2021/04/13 23:25, Kohei KaiGai wrote:
> 2021年4月13日(火) 21:03 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
>> Yeah, ONLY clause is not pushed to the remote server in case of SELECT
>> commands. This is also true for DELETE and UPDATE commands on foreign
>> tables.

This sounds reasonable reason why ONLY should be ignored in TRUNCATE on
foreign tables, for now. If there is the existing rule about how to treat
ONLY clause for foreign tables, basically TRUNCATE should follow that at this
stage. Maybe we can change the rule, but it's an item for v15 or later?


>> I'm not sure if it wasn't thought necessary or if there is an
>> issue to push it or I may be missing something here.

I could not find the past discussion about foreign tables and ONLY clause.
I guess that ONLY is ignored in SELECT on foreign tables case because ONLY
is interpreted outside the executor and it's not easy to change the executor
so that ONLY is passed to FDW. Maybe..


>> I think we can
>> start a separate thread to see other hackers' opinions on this.
>>
>> I'm not sure whether all the clauses that are possible for
>> SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
>> server by postgres_fdw.
>>
>> Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
>> which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
>> If we were to keep it consistent across all foreign commands that
>> ONLY clause is not pushed to remote server, then we can restrict for
>> TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
>> just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
>> don't see any real problem in pushing the ONLY clause, at least in
>> case of TRUNCATE.
>>
> If ONLY-clause would be pushed down to the remote query of postgres_fdw,
> what does the foreign-table represent in the local system?
> 
> In my understanding, a local foreign table by postgres_fdw is a
> representation of
> entire tree of the remote parent table and its children.

If so, I'm still wondering why CASCADE/RESTRICT (i.e., DropBehavior) needs to
be passed to FDW. IOW, if a foreign table is an abstraction of an external
data source, ISTM that postgres_fdw should always issue TRUNCATE with
CASCADE. Why do we need to allow RESTRICT to be specified for a foreign table
even though it's an abstraction of an external data source?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Apr 13, 2021 at 8:30 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/04/13 23:25, Kohei KaiGai wrote:
> > 2021年4月13日(火) 21:03 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
> >> Yeah, ONLY clause is not pushed to the remote server in case of SELECT
> >> commands. This is also true for DELETE and UPDATE commands on foreign
> >> tables.
>
> This sounds reasonable reason why ONLY should be ignored in TRUNCATE on
> foreign tables, for now. If there is the existing rule about how to treat
> ONLY clause for foreign tables, basically TRUNCATE should follow that at this
> stage. Maybe we can change the rule, but it's an item for v15 or later?
>
> >> I'm not sure if it wasn't thought necessary or if there is an
> >> issue to push it or I may be missing something here.
>
> I could not find the past discussion about foreign tables and ONLY clause.
> I guess that ONLY is ignored in SELECT on foreign tables case because ONLY
> is interpreted outside the executor and it's not easy to change the executor
> so that ONLY is passed to FDW. Maybe..
>
>
> >> I think we can
> >> start a separate thread to see other hackers' opinions on this.
> >>
> >> I'm not sure whether all the clauses that are possible for
> >> SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
> >> server by postgres_fdw.
> >>
> >> Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
> >> which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
> >> If we were to keep it consistent across all foreign commands that
> >> ONLY clause is not pushed to remote server, then we can restrict for
> >> TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
> >> just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
> >> don't see any real problem in pushing the ONLY clause, at least in
> >> case of TRUNCATE.
> >>
> > If ONLY-clause would be pushed down to the remote query of postgres_fdw,
> > what does the foreign-table represent in the local system?
> >
> > In my understanding, a local foreign table by postgres_fdw is a
> > representation of
> > entire tree of the remote parent table and its children.
>
> If so, I'm still wondering why CASCADE/RESTRICT (i.e., DropBehavior) needs to
> be passed to FDW. IOW, if a foreign table is an abstraction of an external
> data source, ISTM that postgres_fdw should always issue TRUNCATE with
> CASCADE. Why do we need to allow RESTRICT to be specified for a foreign table
> even though it's an abstraction of an external data source?

IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
RESTART/CONTINUE IDENTITY), because it doesn't have any major
challenge(implementation wise) unlike pushing some clauses in
SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
look good and may confuse users, if we push some options and restrict
others. We should have an explicit note in the documentation saying we
push all these options to the remote server. We can leave it to the
user to write TRUNCATE for foreign tables with the appropriate
options. If somebody complains about a problem that they will face
with this behavior, we can revisit. This is my opinion, others may
disagree.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

2021年4月14日(水) 0:00 Fujii Masao <masao.fujii@oss.nttdata.com>:
>
> On 2021/04/13 23:25, Kohei KaiGai wrote:
> > 2021年4月13日(火) 21:03 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
> >> Yeah, ONLY clause is not pushed to the remote server in case of SELECT
> >> commands. This is also true for DELETE and UPDATE commands on foreign
> >> tables.
>
> This sounds reasonable reason why ONLY should be ignored in TRUNCATE on
> foreign tables, for now. If there is the existing rule about how to treat
> ONLY clause for foreign tables, basically TRUNCATE should follow that at this
> stage. Maybe we can change the rule, but it's an item for v15 or later?
>
>
> >> I'm not sure if it wasn't thought necessary or if there is an
> >> issue to push it or I may be missing something here.
>
> I could not find the past discussion about foreign tables and ONLY clause.
> I guess that ONLY is ignored in SELECT on foreign tables case because ONLY
> is interpreted outside the executor and it's not easy to change the executor
> so that ONLY is passed to FDW. Maybe..
>
>
> >> I think we can
> >> start a separate thread to see other hackers' opinions on this.
> >>
> >> I'm not sure whether all the clauses that are possible for
> >> SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
> >> server by postgres_fdw.
> >>
> >> Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
> >> which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
> >> If we were to keep it consistent across all foreign commands that
> >> ONLY clause is not pushed to remote server, then we can restrict for
> >> TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
> >> just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
> >> don't see any real problem in pushing the ONLY clause, at least in
> >> case of TRUNCATE.
> >>
> > If ONLY-clause would be pushed down to the remote query of postgres_fdw,
> > what does the foreign-table represent in the local system?
> >
> > In my understanding, a local foreign table by postgres_fdw is a
> > representation of
> > entire tree of the remote parent table and its children.
>
> If so, I'm still wondering why CASCADE/RESTRICT (i.e., DropBehavior) needs to
> be passed to FDW. IOW, if a foreign table is an abstraction of an external
> data source, ISTM that postgres_fdw should always issue TRUNCATE with
> CASCADE. Why do we need to allow RESTRICT to be specified for a foreign table
> even though it's an abstraction of an external data source?
>
Please assume the internal heap data is managed by PostgreSQL core, and
external data source is managed by postgres_fdw (or other FDW driver).
TRUNCATE command requires these object managers to eliminate the data
on behalf of the foreign tables picked up.

Even though the object manager tries to eliminate the managed data, it may be
restricted by some reason; FK restrictions in case of PostgreSQL internal data.
In this case, CASCADE/RESTRICT option suggests the object manager how
to handle the target data.

The ONLY clause controls whoes data shall be eliminated.
On the other hand, CASCADE/RESTRICT and CONTINUE/RESTART controls
how data shall be eliminated. It is a primitive difference.

Best regards,
--
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei <kaigai@heterodb.com>

At Wed, 14 Apr 2021 13:17:55 +0900, Kohei KaiGai <kaigai@heterodb.com> wrote in 
> 2021年4月14日(水) 0:00 Fujii Masao <masao.fujii@oss.nttdata.com>:
> >
> > On 2021/04/13 23:25, Kohei KaiGai wrote:
> > > 2021年4月13日(火) 21:03 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>:
> > >> Yeah, ONLY clause is not pushed to the remote server in case of SELECT
> > >> commands. This is also true for DELETE and UPDATE commands on foreign
> > >> tables.
> >
> > This sounds reasonable reason why ONLY should be ignored in TRUNCATE on
> > foreign tables, for now. If there is the existing rule about how to treat
> > ONLY clause for foreign tables, basically TRUNCATE should follow that at this
> > stage. Maybe we can change the rule, but it's an item for v15 or later?
> >
> >
> > >> I'm not sure if it wasn't thought necessary or if there is an
> > >> issue to push it or I may be missing something here.
> >
> > I could not find the past discussion about foreign tables and ONLY clause.
> > I guess that ONLY is ignored in SELECT on foreign tables case because ONLY
> > is interpreted outside the executor and it's not easy to change the executor
> > so that ONLY is passed to FDW. Maybe..
> >
> >
> > >> I think we can
> > >> start a separate thread to see other hackers' opinions on this.
> > >>
> > >> I'm not sure whether all the clauses that are possible for
> > >> SELECT/UPDATE/DELETE/INSERT with local tables are pushed to the remote
> > >> server by postgres_fdw.
> > >>
> > >> Well, now foreign TRUNCATE pushes the ONLY clause to the remote server
> > >> which is inconsistent when compared to SELECT/UPDATE/DELETE commands.
> > >> If we were to keep it consistent across all foreign commands that
> > >> ONLY clause is not pushed to remote server, then we can restrict for
> > >> TRUNCATE too and even if "TRUNCATE ONLY foreign_tbl" is specified,
> > >> just pass "TRUNCATE foreign_tbl" to remote server. Having said that, I
> > >> don't see any real problem in pushing the ONLY clause, at least in
> > >> case of TRUNCATE.
> > >>
> > > If ONLY-clause would be pushed down to the remote query of postgres_fdw,
> > > what does the foreign-table represent in the local system?
> > >
> > > In my understanding, a local foreign table by postgres_fdw is a
> > > representation of
> > > entire tree of the remote parent table and its children.
> >
> > If so, I'm still wondering why CASCADE/RESTRICT (i.e., DropBehavior) needs to
> > be passed to FDW. IOW, if a foreign table is an abstraction of an external
> > data source, ISTM that postgres_fdw should always issue TRUNCATE with
> > CASCADE. Why do we need to allow RESTRICT to be specified for a foreign table
> > even though it's an abstraction of an external data source?
> >
> Please assume the internal heap data is managed by PostgreSQL core, and
> external data source is managed by postgres_fdw (or other FDW driver).
> TRUNCATE command requires these object managers to eliminate the data
> on behalf of the foreign tables picked up.
> 
> Even though the object manager tries to eliminate the managed data, it may be
> restricted by some reason; FK restrictions in case of PostgreSQL internal data.
> In this case, CASCADE/RESTRICT option suggests the object manager how
> to handle the target data.
> 
> The ONLY clause controls whoes data shall be eliminated.
> On the other hand, CASCADE/RESTRICT and CONTINUE/RESTART controls
> how data shall be eliminated. It is a primitive difference.

I object to unconditionally push ONLY to remote. As Kaigai-san said
that it works an apparent wrong way when a user wants to truncate only
the specified foreign table in a inheritance tree and there's no way to
avoid the behavior.

I also don't think it is right to push down CASCADE/RESTRICT.  The
options suggest to propagate truncation to *local* referrer tables
from the *foreign* table, not to the remote referrer tables from the
original table on remote. If a user want to allow that behavior it
should be specified by foreign table options.  (It is bothersome when
someone wants to specify the behavior on-the-fly.)

alter foreign table ft1 options (add truncate_cascade 'true');

Also, CONTINUE/RESTART IDENTITY should not work since foreign tables
don't have an identity-sequence. However, this we might be able to
push down the options since it affects only the target table.

I would accept that behavior if TRUNCATE were "TRUNCATE FOREIGN
TABLE", which explicitly targets a foreign table. But I'm not sure it
is possible to add such syntax reasonable way.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2021/04/14 12:54, Bharath Rupireddy wrote:
> IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
> RESTART/CONTINUE IDENTITY), because it doesn't have any major
> challenge(implementation wise) unlike pushing some clauses in
> SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
> look good and may confuse users, if we push some options and restrict
> others. We should have an explicit note in the documentation saying we
> push all these options to the remote server. We can leave it to the
> user to write TRUNCATE for foreign tables with the appropriate
> options. If somebody complains about a problem that they will face
> with this behavior, we can revisit.

That's one of the options. But I'm afraid it's hard to drop (revisit)
the feature once it has been released. So if there is no explicit
use case for that, basically I'd like to drop that before release
like we agree to drop unused TRUNCATE_REL_CONTEXT_CASCADING.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/14 13:41, Kyotaro Horiguchi wrote:
> At Wed, 14 Apr 2021 13:17:55 +0900, Kohei KaiGai <kaigai@heterodb.com> wrote in
>> Please assume the internal heap data is managed by PostgreSQL core, and
>> external data source is managed by postgres_fdw (or other FDW driver).
>> TRUNCATE command requires these object managers to eliminate the data
>> on behalf of the foreign tables picked up.
>>
>> Even though the object manager tries to eliminate the managed data, it may be
>> restricted by some reason; FK restrictions in case of PostgreSQL internal data.
>> In this case, CASCADE/RESTRICT option suggests the object manager how
>> to handle the target data.
>>
>> The ONLY clause controls whoes data shall be eliminated.
>> On the other hand, CASCADE/RESTRICT and CONTINUE/RESTART controls
>> how data shall be eliminated. It is a primitive difference.

I have a different view on this classification. IMO ONLY and RESTRICT/CASCADE
should be categorized into the same group. Because both options specify
whether to truncate dependent tables or not. If we treat a foreign table as
an abstraction of external data source, ISTM that we should not take care of
table dependancy in the remote server. IOW, we should truncate entire
external data source, i.e., postgres_fdw should push neither ONLY nor
RESTRICT down to the remote server.


> I object to unconditionally push ONLY to remote. As Kaigai-san said
> that it works an apparent wrong way when a user wants to truncate only
> the specified foreign table in a inheritance tree and there's no way to
> avoid the behavior.
> 
> I also don't think it is right to push down CASCADE/RESTRICT.  The
> options suggest to propagate truncation to *local* referrer tables
> from the *foreign* table, not to the remote referrer tables from the
> original table on remote.

Agreed.


> If a user want to allow that behavior it
> should be specified by foreign table options.  (It is bothersome when
> someone wants to specify the behavior on-the-fly.)
> 
> alter foreign table ft1 options (add truncate_cascade 'true');

Maybe. I think this is the item for v15 or later.


> Also, CONTINUE/RESTART IDENTITY should not work since foreign tables
> don't have an identity-sequence. However, this we might be able to
> push down the options since it affects only the target table.

+1

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2021/04/14 12:54, Bharath Rupireddy wrote:
> > IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
> > RESTART/CONTINUE IDENTITY), because it doesn't have any major
> > challenge(implementation wise) unlike pushing some clauses in
> > SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
> > look good and may confuse users, if we push some options and restrict
> > others. We should have an explicit note in the documentation saying we
> > push all these options to the remote server. We can leave it to the
> > user to write TRUNCATE for foreign tables with the appropriate
> > options. If somebody complains about a problem that they will face
> > with this behavior, we can revisit.
>
> That's one of the options. But I'm afraid it's hard to drop (revisit)
> the feature once it has been released. So if there is no explicit
> use case for that, basically I'd like to drop that before release
> like we agree to drop unused TRUNCATE_REL_CONTEXT_CASCADING.

Thanks. Looks like the decision is going in the direction of
restricting those options, I will withdraw my point.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/16 9:15, Bharath Rupireddy wrote:
> On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> On 2021/04/14 12:54, Bharath Rupireddy wrote:
>>> IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
>>> RESTART/CONTINUE IDENTITY), because it doesn't have any major
>>> challenge(implementation wise) unlike pushing some clauses in
>>> SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
>>> look good and may confuse users, if we push some options and restrict
>>> others. We should have an explicit note in the documentation saying we
>>> push all these options to the remote server. We can leave it to the
>>> user to write TRUNCATE for foreign tables with the appropriate
>>> options. If somebody complains about a problem that they will face
>>> with this behavior, we can revisit.
>>
>> That's one of the options. But I'm afraid it's hard to drop (revisit)
>> the feature once it has been released. So if there is no explicit
>> use case for that, basically I'd like to drop that before release
>> like we agree to drop unused TRUNCATE_REL_CONTEXT_CASCADING.
> 
> Thanks. Looks like the decision is going in the direction of
> restricting those options, I will withdraw my point.

We are still discussing whether RESTRICT option should be pushed down to
a foreign data wrapper. But ISTM at least we could reach the consensus about
the drop of extra information for each foreign table. So what about applying
the attached patch and remove the extra information at first?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Fri, 16 Apr 2021 11:54:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> On 2021/04/16 9:15, Bharath Rupireddy wrote:
> > On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao <masao.fujii@oss.nttdata.com>
> > wrote:
> >> On 2021/04/14 12:54, Bharath Rupireddy wrote:
> >>> IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
> >>> RESTART/CONTINUE IDENTITY), because it doesn't have any major
> >>> challenge(implementation wise) unlike pushing some clauses in
> >>> SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
> >>> look good and may confuse users, if we push some options and restrict
> >>> others. We should have an explicit note in the documentation saying we
> >>> push all these options to the remote server. We can leave it to the
> >>> user to write TRUNCATE for foreign tables with the appropriate
> >>> options. If somebody complains about a problem that they will face
> >>> with this behavior, we can revisit.
> >>
> >> That's one of the options. But I'm afraid it's hard to drop (revisit)
> >> the feature once it has been released. So if there is no explicit
> >> use case for that, basically I'd like to drop that before release
> >> like we agree to drop unused TRUNCATE_REL_CONTEXT_CASCADING.
> > Thanks. Looks like the decision is going in the direction of
> > restricting those options, I will withdraw my point.
> 
> We are still discussing whether RESTRICT option should be pushed down to
> a foreign data wrapper. But ISTM at least we could reach the consensus about
> the drop of extra information for each foreign table. So what about applying
> the attached patch and remove the extra information at first?

I'm fine with that direction. Thanks for the patch.

The change is straight-forward and looks fine, except the following
part.

==== contrib/postgres_fdw/sql/postgres_fdw.sql: 2436 -- after patching
2436> -- in case when remote table has inherited children
2437> CREATE TABLE tru_rtable0_child () INHERITS (tru_rtable0);
2438> INSERT INTO tru_rtable0 (SELECT x FROM generate_series(5,9) x);
2439> INSERT INTO tru_rtable0_child (SELECT x FROM generate_series(10,14) x);
2440> SELECT sum(id) FROM tru_ftable;   -- 95
2441>
2442> TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
2443> SELECT count(*) FROM tru_ftable;   -- 0
2444>
2445> INSERT INTO tru_rtable0 (SELECT x FROM generate_series(21,25) x);
2446> SELECT sum(id) FROM tru_ftable;        -- 115
2447> TRUNCATE tru_ftable;            -- truncate both of parent and child
2448> SELECT count(*) FROM tru_ftable;    -- 0

L2445-L2448 doesn't work as described since L2445 inserts tuples only
to the parent.

And there's a slight difference for no reason between the comment at
2442 and 2447.

(The attached is a fix on top of the proposed patch.)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1a3f5cb4ad..d32f291089 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8388,7 +8388,7 @@ SELECT sum(id) FROM tru_ftable;   -- 95
   95
 (1 row)
 
-TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
+TRUNCATE ONLY tru_ftable;        -- truncate both of parent and child
 SELECT count(*) FROM tru_ftable;   -- 0
  count 
 -------
@@ -8396,10 +8396,11 @@ SELECT count(*) FROM tru_ftable;   -- 0
 (1 row)
 
 INSERT INTO tru_rtable0 (SELECT x FROM generate_series(21,25) x);
-SELECT sum(id) FROM tru_ftable;        -- 115
+INSERT INTO tru_rtable0_child (SELECT x FROM generate_series(26,30) x);
+SELECT sum(id) FROM tru_ftable;        -- 255
  sum 
 -----
- 115
+ 255
 (1 row)
 
 TRUNCATE tru_ftable;            -- truncate both of parent and child
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 97c156a472..65643e120d 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2439,11 +2439,12 @@ INSERT INTO tru_rtable0 (SELECT x FROM generate_series(5,9) x);
 INSERT INTO tru_rtable0_child (SELECT x FROM generate_series(10,14) x);
 SELECT sum(id) FROM tru_ftable;   -- 95
 
-TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
+TRUNCATE ONLY tru_ftable;        -- truncate both of parent and child
 SELECT count(*) FROM tru_ftable;   -- 0
 
 INSERT INTO tru_rtable0 (SELECT x FROM generate_series(21,25) x);
-SELECT sum(id) FROM tru_ftable;        -- 115
+INSERT INTO tru_rtable0_child (SELECT x FROM generate_series(26,30) x);
+SELECT sum(id) FROM tru_ftable;        -- 255
 TRUNCATE tru_ftable;            -- truncate both of parent and child
 SELECT count(*) FROM tru_ftable;    -- 0

On Fri, Apr 16, 2021 at 8:24 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> We are still discussing whether RESTRICT option should be pushed down to
> a foreign data wrapper. But ISTM at least we could reach the consensus about
> the drop of extra information for each foreign table. So what about applying
> the attached patch and remove the extra information at first?

Thanks for the patch, here are some comments:

1) Maybe new empty lines would be better so that the code doesn't look
cluttered:
        relids = lappend_oid(relids, myrelid);   --> a new line after this.
        /* Log this relation only if needed for logical decoding */
        if (RelationIsLogicallyLogged(rel))

        relids = lappend_oid(relids, childrelid);  --> a new line after this.
        /* Log this relation only if needed for logical decoding */

        relids = lappend_oid(relids, relid);  --> a new line after this.
        /* Log this relation only if needed for logical decoding */
        if (RelationIsLogicallyLogged(rel))

2) Instead of
     on foreign tables.  <literal>rels</literal> is the list of
     <structname>Relation</structname> data structure that indicates
     a foreign table to truncate.

I think it is better with:
     on foreign tables.  <literal>rels</literal> is the list of
     <structname>Relation</structname> data structures, where each
     entry indicates a foreign table to truncate.

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

4) Isn't it better to mention the "ONLY" option is not pushed to remote
-- truncate with ONLY clause
TRUNCATE ONLY tru_ftable_parent;

TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
SELECT count(*) FROM tru_ftable;   -- 0

5) I may be missing something here, why is even after ONLY is ignored
in the below truncate command, the sum is 126? Shouldn't it truncate
both tru_ftable_parent and
-- truncate with ONLY clause
TRUNCATE ONLY tru_ftable_parent;
SELECT sum(id) FROM tru_ftable_parent;  -- 126

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/16 14:20, Kyotaro Horiguchi wrote:
> At Fri, 16 Apr 2021 11:54:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>> On 2021/04/16 9:15, Bharath Rupireddy wrote:
>>> On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao <masao.fujii@oss.nttdata.com>
>>> wrote:
>>>> On 2021/04/14 12:54, Bharath Rupireddy wrote:
>>>>> IMHO, we can push all the TRUNCATE options (ONLY, RESTRICTED, CASCADE,
>>>>> RESTART/CONTINUE IDENTITY), because it doesn't have any major
>>>>> challenge(implementation wise) unlike pushing some clauses in
>>>>> SELECT/UPDATE/DELETE and we already do this on the master. It doesn't
>>>>> look good and may confuse users, if we push some options and restrict
>>>>> others. We should have an explicit note in the documentation saying we
>>>>> push all these options to the remote server. We can leave it to the
>>>>> user to write TRUNCATE for foreign tables with the appropriate
>>>>> options. If somebody complains about a problem that they will face
>>>>> with this behavior, we can revisit.
>>>>
>>>> That's one of the options. But I'm afraid it's hard to drop (revisit)
>>>> the feature once it has been released. So if there is no explicit
>>>> use case for that, basically I'd like to drop that before release
>>>> like we agree to drop unused TRUNCATE_REL_CONTEXT_CASCADING.
>>> Thanks. Looks like the decision is going in the direction of
>>> restricting those options, I will withdraw my point.
>>
>> We are still discussing whether RESTRICT option should be pushed down to
>> a foreign data wrapper. But ISTM at least we could reach the consensus about
>> the drop of extra information for each foreign table. So what about applying
>> the attached patch and remove the extra information at first?
> 
> I'm fine with that direction. Thanks for the patch.
> 
> The change is straight-forward and looks fine, except the following
> part.
> 
> ==== contrib/postgres_fdw/sql/postgres_fdw.sql: 2436 -- after patching
> 2436> -- in case when remote table has inherited children
> 2437> CREATE TABLE tru_rtable0_child () INHERITS (tru_rtable0);
> 2438> INSERT INTO tru_rtable0 (SELECT x FROM generate_series(5,9) x);
> 2439> INSERT INTO tru_rtable0_child (SELECT x FROM generate_series(10,14) x);
> 2440> SELECT sum(id) FROM tru_ftable;   -- 95
> 2441>
> 2442> TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
> 2443> SELECT count(*) FROM tru_ftable;   -- 0
> 2444>
> 2445> INSERT INTO tru_rtable0 (SELECT x FROM generate_series(21,25) x);
> 2446> SELECT sum(id) FROM tru_ftable;        -- 115
> 2447> TRUNCATE tru_ftable;            -- truncate both of parent and child
> 2448> SELECT count(*) FROM tru_ftable;    -- 0
> 
> L2445-L2448 doesn't work as described since L2445 inserts tuples only
> to the parent.
> 
> And there's a slight difference for no reason between the comment at
> 2442 and 2447.

Agreed. Thanks!


> (The attached is a fix on top of the proposed patch.)

I will include this patch into the main patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/04/16 15:13, Bharath Rupireddy wrote:
> On Fri, Apr 16, 2021 at 8:24 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> We are still discussing whether RESTRICT option should be pushed down to
>> a foreign data wrapper. But ISTM at least we could reach the consensus about
>> the drop of extra information for each foreign table. So what about applying
>> the attached patch and remove the extra information at first?
> 
> Thanks for the patch, here are some comments:

Thanks for the review!

> 
> 1) Maybe new empty lines would be better so that the code doesn't look
> cluttered:
>          relids = lappend_oid(relids, myrelid);   --> a new line after this.
>          /* Log this relation only if needed for logical decoding */
>          if (RelationIsLogicallyLogged(rel))
> 
>          relids = lappend_oid(relids, childrelid);  --> a new line after this.
>          /* Log this relation only if needed for logical decoding */
> 
>          relids = lappend_oid(relids, relid);  --> a new line after this.
>          /* Log this relation only if needed for logical decoding */
>          if (RelationIsLogicallyLogged(rel))

Applied. Attached is the updated version of the patch
(truncate_foreign_table_dont_pass_only_clause_v2.patch).
This patch includes the patch that Horiguchi-san posted upthead.
I'm thinking to commit this patch at first.



> 2) Instead of
>       on foreign tables.  <literal>rels</literal> is the list of
>       <structname>Relation</structname> data structure that indicates
>       a foreign table to truncate.
> 
> I think it is better with:
>       on foreign tables.  <literal>rels</literal> is the list of
>       <structname>Relation</structname> data structures, where each
>       entry indicates a foreign table to truncate.

Justin posted the patch that improves the documents including
this description. I think that we should revisit that patch.
Attached is the updated version of that patch.
(truncate_foreign_table_docs_v1.patch)


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


> 4) Isn't it better to mention the "ONLY" option is not pushed to remote
> -- truncate with ONLY clause
> TRUNCATE ONLY tru_ftable_parent;
> 
> TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
> SELECT count(*) FROM tru_ftable;   -- 0
> 
> 5) I may be missing something here, why is even after ONLY is ignored
> in the below truncate command, the sum is 126? Shouldn't it truncate
> both tru_ftable_parent and
> -- truncate with ONLY clause
> TRUNCATE ONLY tru_ftable_parent;
> SELECT sum(id) FROM tru_ftable_parent;  -- 126

Because TRUNCATE ONLY command doesn't truncate tru_ftable_child talbe
that inehrits tru_ftable_parent. No?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Apr 21, 2021 at 8:31 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> Applied. Attached is the updated version of the patch
> (truncate_foreign_table_dont_pass_only_clause_v2.patch).
> This patch includes the patch that Horiguchi-san posted upthead.
> I'm thinking to commit this patch at first.

+1.

> > 2) Instead of
> >       on foreign tables.  <literal>rels</literal> is the list of
> >       <structname>Relation</structname> data structure that indicates
> >       a foreign table to truncate.
> >
> > I think it is better with:
> >       on foreign tables.  <literal>rels</literal> is the list of
> >       <structname>Relation</structname> data structures, where each
> >       entry indicates a foreign table to truncate.
>
> Justin posted the patch that improves the documents including
> this description. I think that we should revisit that patch.
> Attached is the updated version of that patch.
> (truncate_foreign_table_docs_v1.patch)

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

Other than above, the patch LGTM.

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

> > 4) Isn't it better to mention the "ONLY" option is not pushed to remote
> > -- truncate with ONLY clause
> > TRUNCATE ONLY tru_ftable_parent;
> >
> > TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
> > SELECT count(*) FROM tru_ftable;   -- 0
> >
> > 5) I may be missing something here, why is even after ONLY is ignored
> > in the below truncate command, the sum is 126? Shouldn't it truncate
> > both tru_ftable_parent and
> > -- truncate with ONLY clause
> > TRUNCATE ONLY tru_ftable_parent;
> > SELECT sum(id) FROM tru_ftable_parent;  -- 126
>
> Because TRUNCATE ONLY command doesn't truncate tru_ftable_child talbe
> that inehrits tru_ftable_parent. No?

I get it. tru_ftable_child is a local child so ONLY doesn't truncate it.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

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.


> Other than above, the patch LGTM.
> 
>>> 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.


>>> 4) Isn't it better to mention the "ONLY" option is not pushed to remote
>>> -- truncate with ONLY clause
>>> TRUNCATE ONLY tru_ftable_parent;
>>>
>>> TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
>>> SELECT count(*) FROM tru_ftable;   -- 0

I added the comment.


Could you review the attached patches?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Thu, Apr 22, 2021 at 03:36:25PM +0900, Fujii Masao wrote:
> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
> index 553524553b..69aa66e73e 100644
> --- a/doc/src/sgml/fdwhandler.sgml
> +++ b/doc/src/sgml/fdwhandler.sgml
> @@ -1076,27 +1076,25 @@ ExecForeignTruncate(List *rels,
>                      bool restart_seqs);
>      <para>
> -     <literal>behavior</literal> defines how foreign tables should
> -     be truncated, using as possible values <literal>DROP_RESTRICT</literal>,
> -     which means that <literal>RESTRICT</literal> option is specified,
> -     and <literal>DROP_CASCADE</literal>, which means that
> -     <literal>CASCADE</literal> option is specified, in
> -     <command>TRUNCATE</command> command.
> +     <literal>behavior</literal> is either <literal>DROP_RESTRICT</literal>
> +     or <literal>DROP_CASCADE</literal>, which indicates that the
> +     <literal>RESTRICT</literal> or <literal>CASCADE</literal> option was
> +     requested in the original <command>TRUNCATE</command> command,
> +     respectively.

Now that I reread this, I would change "which indicates" to "indicating".

> -     <literal>restart_seqs</literal> is set to <literal>true</literal>
> -     if <literal>RESTART IDENTITY</literal> option is specified in
> -     <command>TRUNCATE</command> command.  It is <literal>false</literal>
> -     if <literal>CONTINUE IDENTITY</literal> option is specified.
> +     If <literal>restart_seqs</literal> is <literal>true</literal>,
> +     the original <command>TRUNCATE</command> command requested the
> +     <literal>RESTART IDENTITY</literal> option, otherwise
> +     <literal>CONTINUE IDENTITY</literal> option.

should it say "specified" instead of requested ?
Or should it say "requested the RESTART IDENTITY behavior" ?

Also, I think it should say "..otherwise, the CONTINUE IDENTITY behavior was
requested".

> +++ b/doc/src/sgml/ref/truncate.sgml
> @@ -173,7 +173,7 @@ TRUNCATE [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [
>  
>    <para>
>     <command>TRUNCATE</command> can be used for foreign tables if
> -   the foreign data wrapper supports, for instance,
> +   supported by the foreign data wrapper, for instance,
>     see <xref linkend="postgres-fdw"/>.

what does "for instance" mean here?  I think it should be removed.

> +++ b/doc/src/sgml/fdwhandler.sgml
> @@ -1111,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra,
>       if <literal>CONTINUE IDENTITY</literal> option is specified.
>      </para>
>  
> +    <para>
> +     Note that information about <literal>ONLY</literal> options specified
> +     in the original <command>TRUNCATE</command> command is not passed to
> +     <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

There's an extra space before DELETE

> diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
> index 5320accf6f..d03731b7d4 100644
> --- a/doc/src/sgml/postgres-fdw.sgml
> +++ b/doc/src/sgml/postgres-fdw.sgml
> @@ -69,6 +69,13 @@
>    have privileges to do these things.)
>   </para>
>  
> + <para>
> +  Note that <literal>ONLY</literal> option specified in

add "the" to say: "the ONLY"

> +  <command>SELECT</command>, <command>UPDATE</command>,
> +  <command>DELETE</command> or <command>TRUNCATE</command>
> +  has no effect when accessing or modifyung the remote table.

modifying

-- 
Justin

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.

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

+     <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"?

> >>> 4) Isn't it better to mention the "ONLY" option is not pushed to remote
> >>> -- truncate with ONLY clause
> >>> TRUNCATE ONLY tru_ftable_parent;
> >>>
> >>> TRUNCATE ONLY tru_ftable;        -- truncate both parent and child
> >>> SELECT count(*) FROM tru_ftable;   -- 0
>
> I added the comment.

LGTM.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Apr 22, 2021 at 2:26 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Apr 22, 2021 at 03:36:25PM +0900, Fujii Masao wrote:
> > diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
> > index 553524553b..69aa66e73e 100644
> > --- a/doc/src/sgml/fdwhandler.sgml
> > +++ b/doc/src/sgml/fdwhandler.sgml
> > @@ -1076,27 +1076,25 @@ ExecForeignTruncate(List *rels,
> >                      bool restart_seqs);
> >      <para>
> > -     <literal>behavior</literal> defines how foreign tables should
> > -     be truncated, using as possible values <literal>DROP_RESTRICT</literal>,
> > -     which means that <literal>RESTRICT</literal> option is specified,
> > -     and <literal>DROP_CASCADE</literal>, which means that
> > -     <literal>CASCADE</literal> option is specified, in
> > -     <command>TRUNCATE</command> command.
> > +     <literal>behavior</literal> is either <literal>DROP_RESTRICT</literal>
> > +     or <literal>DROP_CASCADE</literal>, which indicates that the
> > +     <literal>RESTRICT</literal> or <literal>CASCADE</literal> option was
> > +     requested in the original <command>TRUNCATE</command> command,
> > +     respectively.
>
> Now that I reread this, I would change "which indicates" to "indicating".

+1.

> > -     <literal>restart_seqs</literal> is set to <literal>true</literal>
> > -     if <literal>RESTART IDENTITY</literal> option is specified in
> > -     <command>TRUNCATE</command> command.  It is <literal>false</literal>
> > -     if <literal>CONTINUE IDENTITY</literal> option is specified.
> > +     If <literal>restart_seqs</literal> is <literal>true</literal>,
> > +     the original <command>TRUNCATE</command> command requested the
> > +     <literal>RESTART IDENTITY</literal> option, otherwise
> > +     <literal>CONTINUE IDENTITY</literal> option.
>
> should it say "specified" instead of requested ?
> Or should it say "requested the RESTART IDENTITY behavior" ?
>
> Also, I think it should say "..otherwise, the CONTINUE IDENTITY behavior was
> requested".

The original TRUNCATE document uses this - "When RESTART IDENTITY is specified"

IMO the following looks better: "If restart_seqs is true, RESTART
IDENTITY was specified in the original TRUNCATE command, otherwise
CONTINUE IDENTITY was specified."

> > +++ b/doc/src/sgml/ref/truncate.sgml
> > @@ -173,7 +173,7 @@ TRUNCATE [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [
> >
> >    <para>
> >     <command>TRUNCATE</command> can be used for foreign tables if
> > -   the foreign data wrapper supports, for instance,
> > +   supported by the foreign data wrapper, for instance,
> >     see <xref linkend="postgres-fdw"/>.
>
> what does "for instance" mean here?  I think it should be removed.

+1.

> > +++ b/doc/src/sgml/fdwhandler.sgml
> > @@ -1111,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra,
> >       if <literal>CONTINUE IDENTITY</literal> option is specified.
> >      </para>
> >
> > +    <para>
> > +     Note that information about <literal>ONLY</literal> options specified
> > +     in the original <command>TRUNCATE</command> command is not passed to
> > +     <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
>
> There's an extra space before DELETE

Good catch! Extra space after "and" and before "<command>".

> > diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
> > index 5320accf6f..d03731b7d4 100644
> > --- a/doc/src/sgml/postgres-fdw.sgml
> > +++ b/doc/src/sgml/postgres-fdw.sgml
> > @@ -69,6 +69,13 @@
> >    have privileges to do these things.)
> >   </para>
> >
> > + <para>
> > +  Note that <literal>ONLY</literal> option specified in
>
> add "the" to say: "the ONLY"

+1.

> > +  <command>SELECT</command>, <command>UPDATE</command>,
> > +  <command>DELETE</command> or <command>TRUNCATE</command>
> > +  has no effect when accessing or modifyung the remote table.
>
> modifying

Good catch!

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/22 17:56, Justin Pryzby wrote:
> On Thu, Apr 22, 2021 at 03:36:25PM +0900, Fujii Masao wrote:
>> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
>> index 553524553b..69aa66e73e 100644
>> --- a/doc/src/sgml/fdwhandler.sgml
>> +++ b/doc/src/sgml/fdwhandler.sgml
>> @@ -1076,27 +1076,25 @@ ExecForeignTruncate(List *rels,
>>                       bool restart_seqs);
>>       <para>
>> -     <literal>behavior</literal> defines how foreign tables should
>> -     be truncated, using as possible values <literal>DROP_RESTRICT</literal>,
>> -     which means that <literal>RESTRICT</literal> option is specified,
>> -     and <literal>DROP_CASCADE</literal>, which means that
>> -     <literal>CASCADE</literal> option is specified, in
>> -     <command>TRUNCATE</command> command.
>> +     <literal>behavior</literal> is either <literal>DROP_RESTRICT</literal>
>> +     or <literal>DROP_CASCADE</literal>, which indicates that the
>> +     <literal>RESTRICT</literal> or <literal>CASCADE</literal> option was
>> +     requested in the original <command>TRUNCATE</command> command,
>> +     respectively.
> 
> Now that I reread this, I would change "which indicates" to "indicating".

Fixed. Thanks for reviewing the patch!
I will post the updated version of the patch later.


> 
>> -     <literal>restart_seqs</literal> is set to <literal>true</literal>
>> -     if <literal>RESTART IDENTITY</literal> option is specified in
>> -     <command>TRUNCATE</command> command.  It is <literal>false</literal>
>> -     if <literal>CONTINUE IDENTITY</literal> option is specified.
>> +     If <literal>restart_seqs</literal> is <literal>true</literal>,
>> +     the original <command>TRUNCATE</command> command requested the
>> +     <literal>RESTART IDENTITY</literal> option, otherwise
>> +     <literal>CONTINUE IDENTITY</literal> option.
> 
> should it say "specified" instead of requested ?
> Or should it say "requested the RESTART IDENTITY behavior" ?
> 
> Also, I think it should say "..otherwise, the CONTINUE IDENTITY behavior was
> requested".

Fixed.

  
>> +++ b/doc/src/sgml/ref/truncate.sgml
>> @@ -173,7 +173,7 @@ TRUNCATE [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [
>>   
>>     <para>
>>      <command>TRUNCATE</command> can be used for foreign tables if
>> -   the foreign data wrapper supports, for instance,
>> +   supported by the foreign data wrapper, for instance,
>>      see <xref linkend="postgres-fdw"/>.
> 
> what does "for instance" mean here?  I think it should be removed.

Removed.


> 
>> +++ b/doc/src/sgml/fdwhandler.sgml
>> @@ -1111,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra,
>>        if <literal>CONTINUE IDENTITY</literal> option is specified.
>>       </para>
>>   
>> +    <para>
>> +     Note that information about <literal>ONLY</literal> options specified
>> +     in the original <command>TRUNCATE</command> command is not passed to
>> +     <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
> 
> There's an extra space before DELETE

Fixed.


> 
>> diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
>> index 5320accf6f..d03731b7d4 100644
>> --- a/doc/src/sgml/postgres-fdw.sgml
>> +++ b/doc/src/sgml/postgres-fdw.sgml
>> @@ -69,6 +69,13 @@
>>     have privileges to do these things.)
>>    </para>
>>   
>> + <para>
>> +  Note that <literal>ONLY</literal> option specified in
> 
> add "the" to say: "the ONLY"

Fixed.


> 
>> +  <command>SELECT</command>, <command>UPDATE</command>,
>> +  <command>DELETE</command> or <command>TRUNCATE</command>
>> +  has no effect when accessing or modifyung the remote table.
> 
> modifying

Fixed.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

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

On Fri, Apr 23, 2021 at 1:39 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > +    <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.

A small typo in the docs patch: It is "are not passed to", instead of
"is not passed to" since we used plural "options". "Note that the ONLY
options specified in the original TRUNCATE command are not passed to"

+     Note that the <literal>ONLY</literal> options specified
      in the original <command>TRUNCATE</command> command is not passed to

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/23 19:56, Bharath Rupireddy wrote:
> On Fri, Apr 23, 2021 at 1:39 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>> +    <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.
> 
> A small typo in the docs patch: It is "are not passed to", instead of
> "is not passed to" since we used plural "options". "Note that the ONLY
> options specified in the original TRUNCATE command are not passed to"
> 
> +     Note that the <literal>ONLY</literal> options specified
>        in the original <command>TRUNCATE</command> command is not passed to

Thanks for the review! I fixed this.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Fri, Apr 23, 2021 at 9:50 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> Thanks for the review! I fixed this.

Thanks for the updated patches.

In docs v4 patch, I think we can combine below two lines into a single line:
+   supported by the foreign data wrapper,
    see <xref linkend="postgres-fdw"/>.

Other than the above minor change, both patches look good to me, I
have no further comments.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/26 13:52, Bharath Rupireddy wrote:
> On Fri, Apr 23, 2021 at 9:50 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> Thanks for the review! I fixed this.
> 
> Thanks for the updated patches.
> 
> In docs v4 patch, I think we can combine below two lines into a single line:
> +   supported by the foreign data wrapper,
>      see <xref linkend="postgres-fdw"/>.

You mean "supported by the foreign data wrapper <xref linkend="postgres-fdw"/>"?

I was thinking that it's better to separate them because postgres_fdw
is just an example of the foreign data wrapper supporting TRUNCATE.
This makes me think again; isn't it better to add "for example" or
"for instance" into after "data wrapper"? That is,

     <command>TRUNCATE</command> can be used for foreign tables if
     supported by the foreign data wrapper, for instance,
     see <xref linkend="postgres-fdw"/>.


> Other than the above minor change, both patches look good to me, I
> have no further comments.

Thanks! I pushed the patch truncate_foreign_table_dont_pass_only_clause_xx.patch, at first.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Apr 27, 2021 at 11:19 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> > In docs v4 patch, I think we can combine below two lines into a single line:
> > +   supported by the foreign data wrapper,
> >      see <xref linkend="postgres-fdw"/>.
>
> You mean "supported by the foreign data wrapper <xref linkend="postgres-fdw"/>"?
>
> I was thinking that it's better to separate them because postgres_fdw
> is just an example of the foreign data wrapper supporting TRUNCATE.
> This makes me think again; isn't it better to add "for example" or
> "for instance" into after "data wrapper"? That is,
>
>      <command>TRUNCATE</command> can be used for foreign tables if
>      supported by the foreign data wrapper, for instance,
>      see <xref linkend="postgres-fdw"/>.

+1.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021/04/27 15:02, Bharath Rupireddy wrote:
> On Tue, Apr 27, 2021 at 11:19 AM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>>> In docs v4 patch, I think we can combine below two lines into a single line:
>>> +   supported by the foreign data wrapper,
>>>       see <xref linkend="postgres-fdw"/>.
>>
>> You mean "supported by the foreign data wrapper <xref linkend="postgres-fdw"/>"?
>>
>> I was thinking that it's better to separate them because postgres_fdw
>> is just an example of the foreign data wrapper supporting TRUNCATE.
>> This makes me think again; isn't it better to add "for example" or
>> "for instance" into after "data wrapper"? That is,
>>
>>       <command>TRUNCATE</command> can be used for foreign tables if
>>       supported by the foreign data wrapper, for instance,
>>       see <xref linkend="postgres-fdw"/>.
> 
> +1.

Pushed. Thanks!

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION