Обсуждение: TRUNCATE on foreign table
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.
I have fixed the patch due to submit it to Commit Fest 2021-03.
regards,
--
------------------
Kazutaka Onishi
Вложения
Hi,
+ if (strcmp(defel->defname, "truncatable") == 0)
+ server_truncatable = defGetBoolean(defel);
+ 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);
+ 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.I have fixed the patch due to submit it to Commit Fest 2021-03.regards,--------------------Kazutaka Onishi
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.CheersOn 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.I have fixed the patch due to submit it to Commit Fest 2021-03.regards,--------------------Kazutaka Onishi
------------------
Kazutaka Onishi
------------------
Kazutaka Onishi
Вложения
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
> 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.
> 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.
> 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.
> 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.
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
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 and the user only uses the local server.
Then the user has to ask the administrator to run TRUNCATE every time.
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.
Anyway, it's more useful if the user can run queries in one place, right?
Do you have any concerns?
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
Hi,
+ <command>TRUNCATE</command> for each foreign server being involved
+ in one <command>TRUNCATE</command> command (note that invocations
+ in one <command>TRUNCATE</command> command (note that invocations
The 'being' in above sentence can be omitted.
+ 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
+ * 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 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
+ 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
Cheers
On Sat, Apr 3, 2021 at 6:46 AM 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).
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
Continuing previous review...
+ 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 extra underscore.
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:
+ 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.
+ * servers involved in the TRUNCATE.
associated to -> associated with
Should the hash table be released at the end of ExecuteTruncateGuts() ?
Cheers
On Sat, Apr 3, 2021 at 7:38 AM Zhihong Yu <zyu@yugabyte.com> wrote:
Hi,+ <command>TRUNCATE</command> for each foreign server being involved
+ in one <command>TRUNCATE</command> command (note that invocationsThe 'being' in above sentence can be omitted.+ the context where the foreign-tables are truncated. It is a list of integers and same length withThere should be a verb between 'and' and same :It is a list of integers and has same length with+ * 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 usesAlternatively:the elements in a hash table. It uses+ 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_ONLYCheersOn Sat, Apr 3, 2021 at 6:46 AM 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).
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 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
On Thu, Apr 8, 2021 at 6:47 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
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 collected and 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 no use 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 the foreign table found first is collected. For example, when "TRUNCATE ft, ONLY ft" is executed, TRUNCATE_REL_CONTEXT_NORMAL is collected and _ONLY is ignored because "ft" is found first. Is this OK? Or we should collect all, e.g., both _NORMAL and _ONLY should be collected in that example? I think that the current approach (i.e., collect the extra info about table found first if the same table is specified multiple times) is good because even local tables are 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 is specified with ONLY, postgres_fdw also issues the TRUNCATE command for the corresponding remote table with ONLY to the 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.
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
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.
Cheers
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 Thu, Apr 22, 2021 at 4:39 AM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
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.
Since 'the only option' is legitimate English phrase, I think the following would be clearer:
Note that the option <literal>ONLY</literal> ...
Cheers
> > + <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