Re: Remove useless tests about TRUNCATE on foreign table
On Tue, 31 May 2022 09:49:40 +0900 Michael Paquier wrote: > On Mon, May 30, 2022 at 05:08:10PM +0900, Michael Paquier wrote: > > Partitions have also some coverage as far as I can see, so I agree > > that it makes little sense to keep the tests you are removing here. > > And done as of 0efa513. Thank you! -- Yugo NAGATA
Re: Remove useless tests about TRUNCATE on foreign table
On Mon, May 30, 2022 at 05:08:10PM +0900, Michael Paquier wrote: > Partitions have also some coverage as far as I can see, so I agree > that it makes little sense to keep the tests you are removing here. And done as of 0efa513. -- Michael signature.asc Description: PGP signature
Re: Remove useless tests about TRUNCATE on foreign table
On Fri, May 27, 2022 at 05:25:43PM +0900, Yugo NAGATA wrote: > --- TRUNCATE doesn't work on foreign tables, either directly or recursively > -TRUNCATE ft2; -- ERROR > -ERROR: foreign-data wrapper "dummy" has no handler > -TRUNCATE fd_pt1; -- ERROR > -ERROR: foreign-data wrapper "dummy" has no handler > DROP TABLE fd_pt1 CASCADE; In the case of this test, fd_pt1 is a normal table that ft2 inherits, so this TRUNCATE command somewhat checks that the TRUNCATE falls back to the foreign table in this case. However, this happens to be tested in postgres_fdw (see around tru_ftable_parent), > --- TRUNCATE doesn't work on foreign tables, either directly or recursively > -TRUNCATE fd_pt2_1; -- ERROR > -ERROR: foreign-data wrapper "dummy" has no handler > -TRUNCATE fd_pt2; -- ERROR > -ERROR: foreign-data wrapper "dummy" has no handler Partitions have also some coverage as far as I can see, so I agree that it makes little sense to keep the tests you are removing here. -- Michael signature.asc Description: PGP signature
Remove useless tests about TRUNCATE on foreign table
Hello, I found that tests for TRUNCATE on foreign tables are left in the foreign_data regression test. Now TRUNCATE on foreign tables are allowed, so I think the tests should be removed. Currently, the results of the test is "ERROR: foreign-data wrapper "dummy" has no handler", but it is just because the foreign table has no handler, not due to TRUNCATE. The patch is attached. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/test/regress/expected/foreign_data.out b/src/test/regress/expected/foreign_data.out index 5bf03680d2..33505352cc 100644 --- a/src/test/regress/expected/foreign_data.out +++ b/src/test/regress/expected/foreign_data.out @@ -1822,11 +1822,6 @@ Server: s0 FDW options: (delimiter ',', quote '"', "be quoted" 'value') Inherits: fd_pt1 --- TRUNCATE doesn't work on foreign tables, either directly or recursively -TRUNCATE ft2; -- ERROR -ERROR: foreign-data wrapper "dummy" has no handler -TRUNCATE fd_pt1; -- ERROR -ERROR: foreign-data wrapper "dummy" has no handler DROP TABLE fd_pt1 CASCADE; NOTICE: drop cascades to foreign table ft2 -- IMPORT FOREIGN SCHEMA @@ -2047,11 +2042,6 @@ ALTER TABLE fd_pt2 ATTACH PARTITION fd_pt2_1 FOR VALUES IN (1); -- ERROR ERROR: child table is missing constraint "fd_pt2chk1" ALTER FOREIGN TABLE fd_pt2_1 ADD CONSTRAINT fd_pt2chk1 CHECK (c1 > 0); ALTER TABLE fd_pt2 ATTACH PARTITION fd_pt2_1 FOR VALUES IN (1); --- TRUNCATE doesn't work on foreign tables, either directly or recursively -TRUNCATE fd_pt2_1; -- ERROR -ERROR: foreign-data wrapper "dummy" has no handler -TRUNCATE fd_pt2; -- ERROR -ERROR: foreign-data wrapper "dummy" has no handler DROP FOREIGN TABLE fd_pt2_1; DROP TABLE fd_pt2; -- foreign table cannot be part of partition tree made of temporary diff --git a/src/test/regress/sql/foreign_data.sql b/src/test/regress/sql/foreign_data.sql index 9dfff66f54..eefb860adc 100644 --- a/src/test/regress/sql/foreign_data.sql +++ b/src/test/regress/sql/foreign_data.sql @@ -746,10 +746,6 @@ ALTER TABLE fd_pt1 RENAME CONSTRAINT fd_pt1chk3 TO f2_check; \d+ fd_pt1 \d+ ft2 --- TRUNCATE doesn't work on foreign tables, either directly or recursively -TRUNCATE ft2; -- ERROR -TRUNCATE fd_pt1; -- ERROR - DROP TABLE fd_pt1 CASCADE; -- IMPORT FOREIGN SCHEMA @@ -833,10 +829,6 @@ ALTER TABLE fd_pt2 ATTACH PARTITION fd_pt2_1 FOR VALUES IN (1); -- ERROR ALTER FOREIGN TABLE fd_pt2_1 ADD CONSTRAINT fd_pt2chk1 CHECK (c1 > 0); ALTER TABLE fd_pt2 ATTACH PARTITION fd_pt2_1 FOR VALUES IN (1); --- TRUNCATE doesn't work on foreign tables, either directly or recursively -TRUNCATE fd_pt2_1; -- ERROR -TRUNCATE fd_pt2; -- ERROR - DROP FOREIGN TABLE fd_pt2_1; DROP TABLE fd_pt2;
Re: TRUNCATE on foreign table
On 2021/04/27 15:02, Bharath Rupireddy wrote: On Tue, Apr 27, 2021 at 11:19 AM Fujii Masao wrote: In docs v4 patch, I think we can combine below two lines into a single line: + supported by the foreign data wrapper, see . You mean "supported by the foreign data wrapper "? 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, TRUNCATE can be used for foreign tables if supported by the foreign data wrapper, for instance, see . +1. Pushed. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: TRUNCATE on foreign table
On Tue, Apr 27, 2021 at 11:19 AM Fujii Masao wrote: > > In docs v4 patch, I think we can combine below two lines into a single line: > > + supported by the foreign data wrapper, > > see . > > You mean "supported by the foreign data wrapper 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, > > TRUNCATE can be used for foreign tables if > supported by the foreign data wrapper, for instance, > see . +1. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: TRUNCATE on foreign table
On 2021/04/26 13:52, Bharath Rupireddy wrote: On Fri, Apr 23, 2021 at 9:50 PM Fujii Masao 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 . You mean "supported by the foreign data wrapper "? 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, TRUNCATE can be used for foreign tables if supported by the foreign data wrapper, for instance, see . 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
Re: TRUNCATE on foreign table
On Fri, Apr 23, 2021 at 9:50 PM Fujii Masao 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 . 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
Re: TRUNCATE on foreign table
On 2021/04/23 19:56, Bharath Rupireddy wrote: On Fri, Apr 23, 2021 at 1:39 PM Fujii Masao wrote: + + Note that information about ONLY options specified + in the original TRUNCATE 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 ONLY options specified in the original TRUNCATE 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 diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index e08441ec8b..8aa7edfe4a 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -1076,27 +1076,25 @@ ExecForeignTruncate(List *rels, bool restart_seqs); - Truncate a set of foreign tables specified in rels. - This function is called when is executed - on foreign tables. rels is the list of - Relation data structure that indicates - a foreign table to truncate. + Truncate foreign tables. This function is called when + is executed on a foreign table. + rels is a list of Relation + data structures of foreign tables to truncate. - behavior defines how foreign tables should - be truncated, using as possible values DROP_RESTRICT, - which means that RESTRICT option is specified, - and DROP_CASCADE, which means that - CASCADE option is specified, in - TRUNCATE command. + behavior is either DROP_RESTRICT + or DROP_CASCADE indicating that the + RESTRICT or CASCADE option was + requested in the original TRUNCATE command, + respectively. - restart_seqs is set to true - if RESTART IDENTITY option is specified in - TRUNCATE command. It is false - if CONTINUE IDENTITY option is specified. + If restart_seqs is true, + the original TRUNCATE command requested the + RESTART IDENTITY behavior, otherwise the + CONTINUE IDENTITY behavior was requested. @@ -1109,11 +1107,10 @@ ExecForeignTruncate(List *rels, - TRUNCATE invokes - ExecForeignTruncate once per foreign server - that foreign tables to truncate belong to. This means that all foreign - tables included in rels must belong to the same - server. + ExecForeignTruncate is invoked once per + foreign server for which foreign tables are to be truncated. + This means that all foreign tables included in rels + must belong to the same server. diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index b0806c1274..839126c4ef 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -459,11 +459,17 @@ OPTIONS (ADD password_required 'false'); This option controls whether postgres_fdw allows - foreign tables to be truncated using TRUNCATE + foreign tables to be truncated using the TRUNCATE command. It can be specified for a foreign table or a foreign server. A table-level option overrides a server-level option. The default is true. + + + Of course, if the remote table is not in fact truncatable, an error + would occur anyway. Use of this option primarily allows the error to + be thrown locally without querying the remote server. + diff --git a/doc/src/sgml/ref/truncate.sgml b/doc/src/sgml/ref/truncate.sgml index acf3633be4..9af42dd008 100644 --- a/doc/src/sgml/ref/truncate.sgml +++ b/doc/src/sgml/ref/truncate.sgml @@ -173,7 +173,7 @@ TRUNCATE [ TABLE ] [ ONLY ] name [ TRUNCATE can be used for foreign tables if - the foreign data wrapper supports, for instance, + supported by the foreign data wrapper, see . diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index bdc4c3620d..7a798530e3 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -2179,24 +2179,19 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs) void deparseTruncateSql(StringInfo buf, List *rels, - List *rels_extra, DropBehavior behavior, bool res
Re: TRUNCATE on foreign table
On Fri, Apr 23, 2021 at 1:39 PM Fujii Masao wrote: > > + > > + Note that information about ONLY options specified > > + in the original TRUNCATE 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 ONLY options specified in the original TRUNCATE command is not passed to With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: TRUNCATE on foreign table
On 2021/04/22 20:27, Bharath Rupireddy wrote: On Thu, Apr 22, 2021 at 12:06 PM Fujii Masao 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" + rels is a list of Relation + data structures for each foreign table to truncated. Fixed. Thanks! How about a slightly changed phrasing like below? + rels is a list of Relation + 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. DELETE, or TRUNCATE. (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! + + Note that information about ONLY options specified + in the original TRUNCATE 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. + ExecForeignTruncate. This is the same behavior as + for the callback functions for SELECT, + UPDATE and DELETE 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 diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 0e2cd3628c..70d89393d9 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -1076,44 +1076,41 @@ ExecForeignTruncate(List *rels, bool restart_seqs); - Truncate a set of foreign tables specified in rels. - This function is called when is executed - on foreign tables. rels is the list of - Relation data structure that indicates - a foreign table to truncate. + Truncate foreign tables. This function is called when + is executed on a foreign table. + rels is a list of Relation + data structures of foreign tables to truncate. - behavior defines how foreign tables should - be truncated, using as possible values DROP_RESTRICT, - which means that RESTRICT option is specified, - and DROP_CASCADE, which means that - CASCADE option is specified, in - TRUNCATE command. + behavior is either DROP_RESTRICT + or DROP_CASCADE indicating that the + RESTRICT or CASCADE option was + requested in the original TRUNCATE command, + respectively. - restart_seqs is set to true - if RESTART IDENTITY option is specified in - TRUNCATE command. It is false - if CONTINUE IDENTITY option is specified. + If restart_seqs is true, + the original TRUNCATE command requested the + RESTART IDENTITY behavior, otherwise the + CONTINUE IDENTITY behavior was requested. - Note that information about ONLY options specified + Note that the ONLY options specified in the original TRUNCATE command is not passed to - ExecForeignTruncate. This is the same behavior as - for the callback functions for SELECT, + ExecForeignTruncate. This behavior is similar to + the callback functions of SELECT, UPDATE and DELETE on a foreign table. - TRUNCATE invokes - ExecForeignTruncate once per foreign server - that foreign tables to truncate belong to. This means that all fore
Re: TRUNCATE on foreign table
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); - behavior defines how foreign tables should - be truncated, using as possible values DROP_RESTRICT, - which means that RESTRICT option is specified, - and DROP_CASCADE, which means that - CASCADE option is specified, in - TRUNCATE command. + behavior is either DROP_RESTRICT + or DROP_CASCADE, which indicates that the + RESTRICT or CASCADE option was + requested in the original TRUNCATE 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. - restart_seqs is set to true - if RESTART IDENTITY option is specified in - TRUNCATE command. It is false - if CONTINUE IDENTITY option is specified. + If restart_seqs is true, + the original TRUNCATE command requested the + RESTART IDENTITY option, otherwise + CONTINUE IDENTITY 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 ] name [ TRUNCATE can be used for foreign tables if - the foreign data wrapper supports, for instance, + supported by the foreign data wrapper, for instance, see . what does "for instance" mean here? I think it should be removed. Removed. +++ b/doc/src/sgml/fdwhandler.sgml @@ -,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra, if CONTINUE IDENTITY option is specified. + + Note that information about ONLY options specified + in the original TRUNCATE command is not passed to + ExecForeignTruncate. This is the same behavior as + for the callback functions for SELECT, + UPDATE and DELETE 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.) + + Note that ONLY option specified in add "the" to say: "the ONLY" Fixed. + SELECT, UPDATE, + DELETE or TRUNCATE + 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
Re: TRUNCATE on foreign table
On Thu, Apr 22, 2021 at 07:41:06AM -0700, Zhihong Yu wrote: > > > > + Note that ONLY 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 ONLY ... I think the ONLY option is better, more clear, and more consistent with the rest of the documentation. There are only ~5 places where we say "the option >OPTION<": | git grep 'the option <' doc/src/sgml/ And at least 150 places where we say "The >OPTION< option" (I'm sure there are some more which are split across lines). | git grep -E 'the <([^>]*)>[^<]* option' doc/src/sgml/ |wc -l -- Justin
Re: TRUNCATE on foreign table
On Thu, Apr 22, 2021 at 4:39 AM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Thu, Apr 22, 2021 at 2:26 PM 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); > > > > > > - behavior defines how foreign tables should > > > - be truncated, using as possible values > DROP_RESTRICT, > > > - which means that RESTRICT option is specified, > > > - and DROP_CASCADE, which means that > > > - CASCADE option is specified, in > > > - TRUNCATE command. > > > + behavior is either > DROP_RESTRICT > > > + or DROP_CASCADE, which indicates that the > > > + RESTRICT or CASCADE option > was > > > + requested in the original TRUNCATE command, > > > + respectively. > > > > Now that I reread this, I would change "which indicates" to "indicating". > > +1. > > > > - restart_seqs is set to true > > > - if RESTART IDENTITY option is specified in > > > - TRUNCATE command. It is > false > > > - if CONTINUE IDENTITY option is specified. > > > + If restart_seqs is true, > > > + the original TRUNCATE command requested the > > > + RESTART IDENTITY option, otherwise > > > + CONTINUE IDENTITY 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 ] class="parameter">name [ > > > > > > > > > TRUNCATE can be used for foreign tables if > > > - the foreign data wrapper supports, for instance, > > > + supported by the foreign data wrapper, for instance, > > > see . > > > > what does "for instance" mean here? I think it should be removed. > > +1. > > > > +++ b/doc/src/sgml/fdwhandler.sgml > > > @@ -,6 +1099,15 @@ ExecForeignTruncate(List *rels, List > *rels_extra, > > > if CONTINUE IDENTITY option is specified. > > > > > > > > > + > > > + Note that information about ONLY options > specified > > > + in the original TRUNCATE command is not > passed to > > > + ExecForeignTruncate. This is the same > behavior as > > > + for the callback functions for SELECT, > > > + UPDATE and DELETE on > > > > There's an extra space before DELETE > > Good catch! Extra space after "and" and before "". > > > > 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.) > > > > > > > > > + > > > + Note that ONLY 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 ONLY ... Cheers > > > > + SELECT, UPDATE, > > > + DELETE or TRUNCATE > > > + has no effect when accessing or modifyung the remote table. > > > > modifying > > Good catch! > > With Regards, > Bharath Rupireddy. > EnterpriseDB: http://www.enterprisedb.com >
Re: TRUNCATE on foreign table
On Thu, Apr 22, 2021 at 05:09:02PM +0530, Bharath Rupireddy wrote: > > 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." This suggests that one of the two options was "specified", but the user maybe didn't specify either, which is why we used the "behavior" language - if neither is "specified" then the default behavior is what was "requested". -- Justin
Re: TRUNCATE on foreign table
On Thu, Apr 22, 2021 at 2:26 PM 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); > > > > - behavior defines how foreign tables should > > - be truncated, using as possible values > > DROP_RESTRICT, > > - which means that RESTRICT option is specified, > > - and DROP_CASCADE, which means that > > - CASCADE option is specified, in > > - TRUNCATE command. > > + behavior is either DROP_RESTRICT > > + or DROP_CASCADE, which indicates that the > > + RESTRICT or CASCADE option was > > + requested in the original TRUNCATE command, > > + respectively. > > Now that I reread this, I would change "which indicates" to "indicating". +1. > > - restart_seqs is set to true > > - if RESTART IDENTITY option is specified in > > - TRUNCATE command. It is false > > - if CONTINUE IDENTITY option is specified. > > + If restart_seqs is true, > > + the original TRUNCATE command requested the > > + RESTART IDENTITY option, otherwise > > + CONTINUE IDENTITY 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 ] > class="parameter">name [ > > > > > > TRUNCATE can be used for foreign tables if > > - the foreign data wrapper supports, for instance, > > + supported by the foreign data wrapper, for instance, > > see . > > what does "for instance" mean here? I think it should be removed. +1. > > +++ b/doc/src/sgml/fdwhandler.sgml > > @@ -,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra, > > if CONTINUE IDENTITY option is specified. > > > > > > + > > + Note that information about ONLY options specified > > + in the original TRUNCATE command is not passed to > > + ExecForeignTruncate. This is the same behavior > > as > > + for the callback functions for SELECT, > > + UPDATE and DELETE on > > There's an extra space before DELETE Good catch! Extra space after "and" and before "". > > 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.) > > > > > > + > > + Note that ONLY option specified in > > add "the" to say: "the ONLY" +1. > > + SELECT, UPDATE, > > + DELETE or TRUNCATE > > + has no effect when accessing or modifyung the remote table. > > modifying Good catch! With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: TRUNCATE on foreign table
On Thu, Apr 22, 2021 at 12:06 PM Fujii Masao 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" > > + rels is a list of Relation > > + data structures for each foreign table to truncated. > > Fixed. Thanks! > > > How about a slightly changed phrasing like below? > > + rels is a list of Relation > > + 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. > >>> DELETE, or TRUNCATE. > >>> (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! + + Note that information about ONLY options specified + in the original TRUNCATE 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." + ExecForeignTruncate. This is the same behavior as + for the callback functions for SELECT, + UPDATE and DELETE 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
Re: TRUNCATE on foreign table
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); > > - behavior defines how foreign tables should > - be truncated, using as possible values DROP_RESTRICT, > - which means that RESTRICT option is specified, > - and DROP_CASCADE, which means that > - CASCADE option is specified, in > - TRUNCATE command. > + behavior is either DROP_RESTRICT > + or DROP_CASCADE, which indicates that the > + RESTRICT or CASCADE option was > + requested in the original TRUNCATE command, > + respectively. Now that I reread this, I would change "which indicates" to "indicating". > - restart_seqs is set to true > - if RESTART IDENTITY option is specified in > - TRUNCATE command. It is false > - if CONTINUE IDENTITY option is specified. > + If restart_seqs is true, > + the original TRUNCATE command requested the > + RESTART IDENTITY option, otherwise > + CONTINUE IDENTITY 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 ] class="parameter">name [ > > > TRUNCATE can be used for foreign tables if > - the foreign data wrapper supports, for instance, > + supported by the foreign data wrapper, for instance, > see . what does "for instance" mean here? I think it should be removed. > +++ b/doc/src/sgml/fdwhandler.sgml > @@ -,6 +1099,15 @@ ExecForeignTruncate(List *rels, List *rels_extra, > if CONTINUE IDENTITY option is specified. > > > + > + Note that information about ONLY options specified > + in the original TRUNCATE command is not passed to > + ExecForeignTruncate. This is the same behavior as > + for the callback functions for SELECT, > + UPDATE and DELETE 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.) > > > + > + Note that ONLY option specified in add "the" to say: "the ONLY" > + SELECT, UPDATE, > + DELETE or TRUNCATE > + has no effect when accessing or modifyung the remote table. modifying -- Justin
Re: TRUNCATE on foreign table
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" + rels is a list of Relation + data structures for each foreign table to truncated. Fixed. Thanks! How about a slightly changed phrasing like below? + rels is a list of Relation + 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. DELETE, or TRUNCATE. (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 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); - Truncate a set of foreign tables specified in rels. - This function is called when is executed - on foreign tables. rels is the list of - Relation data structure that indicates - a foreign table to truncate. + Truncate foreign tables. This function is called when + is executed on a foreign table. + rels is a list of Relation + data structures for each foreign table to be truncated. - behavior defines how foreign tables should - be truncated, using as possible values DROP_RESTRICT, - which means that RESTRICT option is specified, - and DROP_CASCADE, which means that - CASCADE option is specified, in - TRUNCATE command. + behavior is either DROP_RESTRICT + or DROP_CASCADE, which indicates that the + RESTRICT or CASCADE option was + requested in the original TRUNCATE command, + respectively. - restart_seqs is set to true - if RESTART IDENTITY option is specified in - TRUNCATE command. It is false - if CONTINUE IDENTITY option is specified. + If restart_seqs is true, + the original TRUNCATE command requested the + RESTART IDENTITY option, otherwise + CONTINUE IDENTITY option. @@ -1109,11 +1107,10 @@ ExecForeignTruncate(List *rels, - TRUNCATE invokes - ExecForeignTruncate once per foreign server - that foreign tables to truncate belong to. This means that all foreign - tables included in rels must belong to the same - server. + ExecForeignTruncate is invoked once per + foreign server for which foreign tables are to be truncated. + This means that all foreign tables included in rels + must belong to the same server. diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index d03731b7d4..fe1102ceb4 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -459,11 +459,17 @@ OPTIONS (ADD password_required 'false'); This option controls whether postgres_fdw allows - foreign tables to be truncated using TRUNCATE + foreign tables to be truncated using the TRUNCATE command. It can be specified for a foreign table or a foreign server. A table-level option overrides a server-level option. The default is true. + + + Of course, if the remote table is not in fact truncatable, an error + would occur anyway. Use of this option primarily allows the error to + be thrown locally without querying the remote server. + diff --git a/doc/src/sgml/ref/tr
Re: TRUNCATE on foreign table
On Wed, Apr 21, 2021 at 8:31 PM Fujii Masao 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. rels is the list of > > Relation data structure that indicates > > a foreign table to truncate. > > > > I think it is better with: > > on foreign tables. rels is the list of > > Relation 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" + rels is a list of Relation + data structures for each foreign table to truncated. How about a slightly changed phrasing like below? + rels is a list of Relation + 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. > >DELETE, or TRUNCATE. > >(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
Re: TRUNCATE on foreign table
On 2021/04/16 15:13, Bharath Rupireddy wrote:
On Fri, Apr 16, 2021 at 8:24 AM Fujii Masao 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. rels is the list of
Relation data structure that indicates
a foreign table to truncate.
I think it is better with:
on foreign tables. rels is the list of
Relation 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.
DELETE, or TRUNCATE.
(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
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bdc4c3620d..7a798530e3 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2179,24 +2179,19 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List
**retrieved_attrs)
void
deparseTruncateSql(StringInfo buf,
List *rels,
- List *rels_extra,
DropBehavior behavior,
bool restart_seqs)
{
- ListCell *lc1,
- *lc2;
+ ListCell *cell;
appendStringInfoString(buf, "TRUNCATE ");
- forboth(lc1, rels, lc2, rels_extra)
+ foreach(cell, rels)
{
- Relationrel = lfirst(lc1);
- int extra = lfirst_int(lc2);
+ Relationrel = lfirst(cell);
- if (lc1 != list_head(rels))
+ if (cell != list_head(rels))
appendStringInfoString(buf, ", ");
- if (extra & TRUNCATE_REL_CONTEXT_ONLY)
- appendStringInfoString(buf, "ONLY ");
deparseRelation(buf, rel);
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 5070d93239..d32f291089 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8218,13 +8218,13 @@ drop table loc3;
-- test for TRUNCATE
-- ===
CREATE TABLE tru_rtable0 (id int primary key);
-CREATE TABLE tru_rtable1
Re: TRUNCATE on foreign table
On 2021/04/16 14:20, Kyotaro Horiguchi wrote: At Fri, 16 Apr 2021 11:54:16 +0900, Fujii Masao wrote in On 2021/04/16 9:15, Bharath Rupireddy wrote: On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao 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
Re: TRUNCATE on foreign table
On Fri, Apr 16, 2021 at 8:24 AM Fujii Masao 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. rels is the list of
Relation data structure that indicates
a foreign table to truncate.
I think it is better with:
on foreign tables. rels is the list of
Relation 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.
DELETE, or TRUNCATE.
(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
Re: TRUNCATE on foreign table
At Fri, 16 Apr 2021 11:54:16 +0900, Fujii Masao wrote in > On 2021/04/16 9:15, Bharath Rupireddy wrote: > > On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao > > 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
Re: TRUNCATE on foreign table
TableState
*mtstate,
static void postgresExplainDirectModify(ForeignScanState *node,
ExplainState *es);
static void postgresExecForeignTruncate(List *rels,
-
List *rels_extra,
DropBehavior behavior,
bool restart_seqs);
static bool postgresAnalyzeForeignTable(Relation relation,
@@ -2881,7 +2880,6 @@ postgresExplainDirectModify(ForeignScanState *node,
ExplainState *es)
*/
static void
postgresExecForeignTruncate(List *rels,
- List *rels_extra,
DropBehavior behavior,
bool restart_seqs)
{
@@ -2964,7 +2962,7 @@ postgresExecForeignTruncate(List *rels,
/* Construct the TRUNCATE command string */
initStringInfo();
- deparseTruncateSql(, rels, rels_extra, behavior, restart_seqs);
+ deparseTruncateSql(, rels, behavior, restart_seqs);
/* Issue the TRUNCATE command to remote server */
do_sql_command(conn, sql.data);
diff --git a/contrib/postgres_fdw/postgres_fdw.h
b/contrib/postgres_fdw/postgres_fdw.h
index 5d44b75314..9591c0f6c2 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -209,7 +209,6 @@ extern void deparseAnalyzeSql(StringInfo buf, Relation rel,
List
**retrieved_attrs);
extern void deparseTruncateSql(StringInfo buf,
List *rels,
- List *rels_extra,
DropBehavior
behavior,
bool restart_seqs);
extern void deparseStringLiteral(StringInfo buf, const char *val);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 74590089bd..97c156a472 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2355,7 +2355,6 @@ drop table loc3;
-- test for TRUNCATE
-- ===
CREATE TABLE tru_rtable0 (id int primary key);
-CREATE TABLE tru_rtable1 (id int primary key);
CREATE FOREIGN TABLE tru_ftable (id int)
SERVER loopback OPTIONS (table_name 'tru_rtable0');
INSERT INTO tru_rtable0 (SELECT x FROM generate_series(1,10) x);
@@ -2363,6 +2362,7 @@ INSERT INTO tru_rtable0 (SELECT x FROM
generate_series(1,10) x);
CREATE TABLE tru_ptable (id int) PARTITION BY HASH(id);
CREATE TABLE tru_ptable__p0 PARTITION OF tru_ptable
FOR VALUES WITH (MODULUS 2, REMAINDER 0);
+CREATE TABLE tru_rtable1 (id int primary key);
CREATE FOREIGN TABLE tru_ftable__p1 PARTITION OF tru_ptable
FOR VALUES WITH (MODULUS 2, REMAINDER 1)
SERVER loopback OPTIONS (table_name 'tru_rtable1');
@@ -2439,13 +2439,13 @@ 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 only parent portion
-SELECT sum(id) FROM tru_ftable; -- 60
+TRUNCATE ONLY tru_ftable; -- truncate both 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;-- 175
+SELECT sum(id) FROM tru_ftable;-- 115
TRUNCATE tru_ftable; -- truncate both of parent and child
-SELECT count(*) FROM tru_ftable;-- empty
+SELECT count(*) FROM tru_ftable;-- 0
-- cleanup
DROP FOREIGN TABLE tru_ftable_parent, tru_ftable_child,
tru_pk_ftable,tru_ftable__p1,tru_ftable;
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 98882ddab8..3fe373ef00 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1071,28 +1071,16 @@ EndDirectModify(ForeignScanState *node);
void
-ExecForeignTruncate(List *rels, List *rels_extra,
-DropBehavior behavior, bool restart_seqs);
+ExecForeignTruncate(List *rels,
+DropBehavior behavior,
+bool restart_seqs);
Truncate a set of foreign tables specified in rels.
This function is called when is executed
on foreign tables. rels is the list of
Relation data structure that indicates
- a foreign table to truncate. rels_extra the list of
- int value, which delivers extra information about
Re: TRUNCATE on foreign table
On Thu, Apr 15, 2021 at 8:19 PM Fujii Masao 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
Re: TRUNCATE on foreign table
On 2021/04/14 13:41, Kyotaro Horiguchi wrote: At Wed, 14 Apr 2021 13:17:55 +0900, Kohei KaiGai 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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
At Wed, 14 Apr 2021 13:17:55 +0900, Kohei KaiGai wrote in > 2021年4月14日(水) 0:00 Fujii Masao : > > > > On 2021/04/13 23:25, Kohei KaiGai wrote: > > > 2021年4月13日(火) 21:03 Bharath Rupireddy > > > : > > >> 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
Re: TRUNCATE on foreign table
2021年4月14日(水) 0:00 Fujii Masao : > > On 2021/04/13 23:25, Kohei KaiGai wrote: > > 2021年4月13日(火) 21:03 Bharath Rupireddy > > : > >> 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
Re: TRUNCATE on foreign table
On Tue, Apr 13, 2021 at 8:30 PM Fujii Masao wrote: > On 2021/04/13 23:25, Kohei KaiGai wrote: > > 2021年4月13日(火) 21:03 Bharath Rupireddy > > : > >> 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
Re: TRUNCATE on foreign table
On 2021/04/13 23:25, Kohei KaiGai wrote: 2021年4月13日(火) 21:03 Bharath Rupireddy : 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
Re: TRUNCATE on foreign table
2021年4月13日(火) 21:03 Bharath Rupireddy : > > On Tue, Apr 13, 2021 at 2:37 PM Kohei KaiGai 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
Re: TRUNCATE on foreign table
On Tue, Apr 13, 2021 at 2:37 PM Kohei KaiGai 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
Re: TRUNCATE on foreign table
2021年4月13日(火) 16:17 Fujii Masao : > > 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
Re: TRUNCATE on foreign table
At Tue, 13 Apr 2021 16:17:12 +0900, Fujii Masao 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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
On 2021/04/13 12:46, Justin Pryzby wrote: On Tue, Apr 13, 2021 at 12:38:35PM +0900, Fujii Masao wrote: + Relation data structures for each + foreign tables to be truncated. "foreign tables" should be "foreign table" because it follows "each"? Yes, you're right. + + behavior is either + DROP_RESTRICT or DROP_CASCADE. + DROP_CASCADE indicates that the + CASCADE option was specified in the original TRUNCATE 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, behavior is either DROP_RESTRICT or DROP_CASCADE, which indicates that the RESTRICT or CASCADE option was requested in the original TRUNCATE command, respectively. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: TRUNCATE on foreign table
2021年4月9日(金) 23:49 Kohei KaiGai : > > 2021年4月9日(金) 22:51 Fujii Masao : > > > > On 2021/04/09 12:33, Kohei KaiGai wrote: > > > 2021年4月8日(木) 22:14 Fujii Masao : > > >> > > >> 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 discussion 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 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. > > >> > > >> 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. > > >> > > >> 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. > > >> > > > 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
Re: TRUNCATE on foreign table
On Tue, Apr 13, 2021 at 12:38:35PM +0900, Fujii Masao wrote: > + Relation data structures for each > + foreign tables to be truncated. > > "foreign tables" should be "foreign table" because it follows "each"? Yes, you're right. > + > + behavior is either > + DROP_RESTRICT or DROP_CASCADE. > + DROP_CASCADE indicates that the > + CASCADE option was specified in the original > TRUNCATE 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). -- Justin
Re: TRUNCATE on foreign table
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. + Relation data structures for each + foreign tables to be truncated. "foreign tables" should be "foreign table" because it follows "each"? + + behavior is either + DROP_RESTRICT or DROP_CASCADE. + DROP_CASCADE indicates that the + CASCADE option was specified in the original TRUNCATE command. Why did you remove the description for DROP_RESTRICT? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: TRUNCATE on foreign table
On Tue, Apr 13, 2021 at 6:27 AM Justin Pryzby 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 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_CASCADING0x0004 /* 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
Re: TRUNCATE on foreign table
childrelid); -relids_extra = lappend_int(relids_extra, TRUNCATE_REL_CONTEXT_CASCADING); +relids_extra = lappend_int(relids_extra, 0); /* Log this relation only if needed for logical decoding */ if (RelationIsLogicallyLogged(childrel)) relids_logged = lappend_oid(relids_logged, childrelid); diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index b808a07e46..9aa9f3c6c7 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -25,11 +25,7 @@ * These values indicate how a relation was specified as the target to * truncate in TRUNCATE command. */ -#define TRUNCATE_REL_CONTEXT_NORMAL 1 /* specified without ONLY clause */ -#define TRUNCATE_REL_CONTEXT_ONLY 2 /* specified with ONLY clause */ -#define TRUNCATE_REL_CONTEXT_CASCADING 3 /* not specified but truncated - * due to dependency (e.g., - * partition table) */ +#define TRUNCATE_REL_CONTEXT_ONLY 0x01 /* specified with ONLY clause */ struct AlterTableUtilityContext; /* avoid including tcop/utility.h here */ -- 2.17.0 >From 8f332a4c9a8690ade17421528b1d375ddd992cce Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Thu, 8 Apr 2021 10:10:58 -0500 Subject: [PATCH 2/2] WIP doc review: Allow TRUNCATE command to truncate foreign tables. 8ff1c94649f5c9184ac5f07981d8aea9dfd7ac19 --- doc/src/sgml/fdwhandler.sgml | 55 ++ doc/src/sgml/postgres-fdw.sgml | 2 +- doc/src/sgml/ref/truncate.sgml | 2 +- 3 files changed, 25 insertions(+), 34 deletions(-) diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 98882ddab8..69feefe66b 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -1075,46 +1075,37 @@ ExecForeignTruncate(List *rels, List *rels_extra, DropBehavior behavior, bool restart_seqs); - Truncate a set of foreign tables specified in rels. + Truncate foreign tables. This function is called when is executed - on foreign tables. rels is the list of - Relation data structure that indicates - a foreign table to truncate. rels_extra the list of - int value, which delivers extra information about - a foreign table to truncate. Possible values are - TRUNCATE_REL_CONTEXT_NORMAL, which means that - the foreign table is specified WITHOUT ONLY clause - in TRUNCATE, - TRUNCATE_REL_CONTEXT_ONLY, which means that - the foreign table is specified WITH ONLY clause, - and TRUNCATE_REL_CONTEXT_CASCADING, - which means that the foreign table is not specified explicitly, - but will be truncated due to dependency (for example, partition table). - There is one-to-one mapping between rels and - rels_extra. The number of entries is the same - between the two lists. - - - - behavior defines how foreign tables should - be truncated, using as possible values DROP_RESTRICT, - which means that RESTRICT option is specified, - and DROP_CASCADE, which means that - CASCADE option is specified, in + on a foreign table. rels is a list of + Relation data structures for each + foreign tables to be truncated. + rels_extra is a list of int values + of the same length as rels. Each element of + rels_extra is a bitmask of flags and provides extra + information about the corresponding foreign table. Currently, the only + defined flag is TRUNCATE_REL_CONTEXT_ONLY, which means + that the foreign table was specified with the ONLY + clause in the original TRUNCATE command. + + + + behavior is either + DROP_RESTRICT or DROP_CASCADE. + DROP_CASCADE indicates that the + CASCADE option was specified in the original TRUNCATE command. - restart_seqs is set to true - if RESTART IDENTITY option is specified in - TRUNCATE command. It is false - if CONTINUE IDENTITY option is specified. + If restart_seqs is true, + the original TRUNCATE command included the + RESTART IDENTITY option. - TRUNCATE invokes - ExecForeignTruncate once per foreign server - that foreign tables to truncate belong to. This means that all foreign + ExecForeignTruncate is invoked once per foreign server + for which foreign tables are to be truncated. This means that all foreign tables included in rels must belong to the same server. diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 5320accf6f..116434f658 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -452,7 +452,7 @@ OPTIONS (ADD password_required 'false'); This option controls whether postgres_fdw allows - foreign tables to be truncated using TRUNCATE + foreign tables to be truncated using the TRUNCATE command. It can be specified for a foreign tabl
Re: TRUNCATE on foreign table
On 2021/04/09 23:10, Bharath Rupireddy wrote: On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao 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
Re: TRUNCATE on foreign table
On 2021/04/11 19:15, Bharath Rupireddy wrote: On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby wrote: Find attached language fixes. Thanks for the patches. Thanks for the patches! 0001 patch basically looks good to me. + behavior must be specified as + DROP_RESTRICT or DROP_CASCADE. + If specified as DROP_RESTRICT, the + RESTRICT option will be included in the TRUNCATE command. + If specified as DROP_CASCADE, the + CASCADE 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_CASCADING0x0004 /* 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
Re: TRUNCATE on foreign table
On Sun, Apr 11, 2021 at 9:47 AM Justin Pryzby 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_CASCADING0x0004 /* 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
Re: TRUNCATE on foreign table
On Thu, Apr 08, 2021 at 10:14:17PM +0900, Fujii Masao wrote:
> 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 discussion and change the behavior later if necessary.
>
> Pushed! Thank all involved in this development!!
> For record, I attached the final patch I committed.
Find attached language fixes.
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. You could also write:
| Assert(behavior == DROP_RESTRICT || behavior == 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 "&".
src/include/commands/tablecmds.h-#define TRUNCATE_REL_CONTEXT_NORMAL 1 /*
specified without ONLY clause */
src/include/commands/tablecmds.h-#define TRUNCATE_REL_CONTEXT_ONLY 2 /*
specified with ONLY clause */
src/include/commands/tablecmds.h:#define TRUNCATE_REL_CONTEXT_CASCADING 3
/* not specified but truncated
src/include/commands/tablecmds.h-
* due to dependency (e.g.,
src/include/commands/tablecmds.h-
* partition table) */
> +++ b/contrib/postgres_fdw/deparse.c
> @@ -2172,6 +2173,43 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List
> **retrieved_attrs)
> deparseRelation(buf, rel);
> }
>
> +/*
> + * Construct a simple "TRUNCATE rel" statement
> + */
> +void
> +deparseTruncateSql(StringInfo buf,
> +List *rels,
> +List *rels_extra,
> +DropBehavior behavior,
> +bool restart_seqs)
> +{
> + ListCell *lc1,
> +*lc2;
> +
> + appendStringInfoString(buf, "TRUNCATE ");
> +
> + forboth(lc1, rels, lc2, rels_extra)
> + {
> + Relationrel = lfirst(lc1);
> + int extra = lfirst_int(lc2);
> +
> + if (lc1 != list_head(rels))
> + appendStringInfoString(buf, ", ");
> + if (extra & TRUNCATE_REL_CONTEXT_ONLY)
> + appendStringInfoString(buf, "ONLY ");
> +
> + deparseRelation(buf, rel);
> + }
> +
> + appendStringInfo(buf, " %s IDENTITY",
> + restart_seqs ? "RESTART" : "CONTINUE");
> +
> + if (behavior == DROP_RESTRICT)
> + appendStringInfoString(buf, " RESTRICT");
> + else if (behavior == DROP_CASCADE)
> + appendStringInfoString(buf, " CASCADE");
> +}
>From 7bbd9312464899dfc2c70fdc64c95a298ac01594 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Thu, 8 Apr 2021 10:10:58 -0500
Subject: [PATCH 1/3] WIP doc review: Allow TRUNCATE command to truncate
foreign tables.
8ff1c94649f5c9184ac5f07981d8aea9dfd7ac19
---
doc/src/sgml/fdwhandler.sgml | 43 +-
doc/src/sgml/postgres-fdw.sgml | 2 +-
doc/src/sgml/ref/truncate.sgml | 2 +-
3 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 98882ddab8..5a76dede24 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1075,40 +1075,39 @@ ExecForeignTruncate(List *rels, List *rels_extra,
DropBehavior behavior, bool restart_seqs);
- Truncate a set of foreign tables specified in rels.
+ Truncate foreign tables.
This function is called when is executed
- on foreign tables. rels is the list of
- Relation data structure that indicates
- a foreign table to truncate. rels_extra the list of
- int value, which delivers extra information about
- a foreign table to truncate. Possible values are
+ on a foreign table. rels is a list of
+ Relation data structures for the
+ foreign tables to be truncated. rels_extra is a list of
+ corresponding int values which provide extra information about
+ the foreign tables. Each element of rels_extra may have the value
TRUNCATE_REL_CONTEXT_NORMAL, which means that
- the foreign table is specified WITHOUT ONLY clause
+ the foreign table is specified WITHOUT the ONLY clause
in TRUNCATE,
TRUNCATE_REL_CONTEXT_ONLY, which means that
- the for
Re: TRUNCATE on foreign table
2021年4月9日(金) 22:51 Fujii Masao : > > On 2021/04/09 12:33, Kohei KaiGai wrote: > > 2021年4月8日(木) 22:14 Fujii Masao : > >> > >> 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 discussion 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 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. > >> > >> 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. > >> > >> 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. > >> > > 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
Re: TRUNCATE on foreign table
On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao wrote:
> > > 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.
>
> But isn't it difficult for remote server to determine how to execute? Please
> imagine the case where there are four tables as 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 if we collect all the information
> about context, both TRUNCATE_REL_CONTEXT_NORMAL and _ONLY are passed to FDW.
> In this case how should FDW determine whether to use ONLY when issuing
> TRUNCATE command to the remote server? Isn't it difficult to do that? If FDW
> determines not to use ONLY because _NORMAL flag is passed, both remote_parent
> and remote_child tables are truncated. That is, though both local_child and
> remote_child are the inheriting tables, isn't it strange that 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
Re: TRUNCATE on foreign table
On Fri, Apr 9, 2021 at 7:06 PM Fujii Masao 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
Re: TRUNCATE on foreign table
On 2021/04/09 12:33, Kohei KaiGai wrote: 2021年4月8日(木) 22:14 Fujii Masao : 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 discussion 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 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. 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. 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. 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 to FDW. 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
Re: TRUNCATE on foreign table
On 2021/04/09 11:05, Zhihong Yu wrote: On Thu, Apr 8, 2021 at 6:47 PM Bharath Rupireddy mailto:bharath.rupireddyforpostg...@gmail.com>> wrote: On Thu, Apr 8, 2021 at 6:44 PM Fujii Masao mailto:masao.fu...@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. +1 > 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. But isn't it difficult for remote server to determine how to execute? Please imagine the case where there are four tables as 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 if we collect all the information about context, both TRUNCATE_REL_CONTEXT_NORMAL and _ONLY are passed to FDW. In this case how should FDW determine whether to use ONLY when issuing TRUNCATE command to the remote server? Isn't it difficult to do that? If FDW determines not to use ONLY because _NORMAL flag is passed, both remote_parent and remote_child tables are truncated. That is, though both local_child and remote_child are the inheriting tables, isn't it strange that only the former 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 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. +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 diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index 26ac786c51..d34271e3b8 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -549,6 +549,18 @@ static const SchemaQuery Query_for_l
Re: TRUNCATE on foreign table
2021年4月8日(木) 22:14 Fujii Masao : > > 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 discussion 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 > 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. > > 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. > > 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. > 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
Re: TRUNCATE on foreign table
On Thu, Apr 8, 2021 at 6:47 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Thu, Apr 8, 2021 at 6:44 PM Fujii Masao > 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
Re: TRUNCATE on foreign table
On Thu, Apr 8, 2021 at 6:44 PM Fujii Masao 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
Re: TRUNCATE on foreign table
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 discussion and 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 : > > > > 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 discussion 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 > 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. > > 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. > > 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. > > 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
Re: TRUNCATE on foreign table
reign tables as partitions +SELECT sum(id) FROM tru_ptable;-- 155 +TRUNCATE tru_ptable; +SELECT count(*) FROM tru_ptable; -- 0 +SELECT count(*) FROM tru_ptable__p0; -- 0 +SELECT count(*) FROM tru_ftable__p1; -- 0 +SELECT count(*) FROM tru_rtable1; -- 0 + +-- 'CASCADE' option +SELECT sum(id) FROM tru_pk_ftable; -- 55 +TRUNCATE tru_pk_ftable;-- failed by FK reference +TRUNCATE tru_pk_ftable CASCADE; +SELECT count(*) FROM tru_pk_ftable;-- 0 +SELECT count(*) FROM tru_fk_table; -- also truncated,0 + +-- truncate two tables at a command +INSERT INTO tru_ftable (SELECT x FROM generate_series(1,8) x); +INSERT INTO tru_pk_ftable (SELECT x FROM generate_series(3,10) x); +SELECT count(*) from tru_ftable; -- 8 +SELECT count(*) from tru_pk_ftable; -- 8 +TRUNCATE tru_ftable, tru_pk_ftable CASCADE; +SELECT count(*) from tru_ftable; -- 0 +SELECT count(*) from tru_pk_ftable; -- 0 + +-- truncate with ONLY clause +TRUNCATE ONLY tru_ftable_parent; +SELECT sum(id) FROM tru_ftable_parent; -- 126 +TRUNCATE tru_ftable_parent; +SELECT count(*) FROM tru_ftable_parent; -- 0 + +-- in case when remote table has inherited children +CREATE TABLE tru_rtable0_child () INHERITS (tru_rtable0); +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 only parent portion +SELECT sum(id) FROM tru_ftable; -- 60 + +INSERT INTO tru_rtable0 (SELECT x FROM generate_series(21,25) x); +SELECT sum(id) FROM tru_ftable;-- 175 +TRUNCATE tru_ftable; -- truncate both of parent and child +SELECT count(*) FROM tru_ftable;-- empty + +-- cleanup +DROP FOREIGN TABLE tru_ftable_parent, tru_ftable_child, tru_pk_ftable,tru_ftable__p1,tru_ftable; +DROP TABLE tru_rtable0, tru_rtable1, tru_ptable, tru_ptable__p0, tru_pk_table, tru_fk_table, +tru_rtable_parent,tru_rtable_child, tru_rtable0_child; + -- === -- test IMPORT FOREIGN SCHEMA -- === diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 0f2397df49..98882ddab8 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -1065,6 +1065,67 @@ EndDirectModify(ForeignScanState *node); + +FDW Routines for TRUNCATE + + + +void +ExecForeignTruncate(List *rels, List *rels_extra, +DropBehavior behavior, bool restart_seqs); + + + Truncate a set of foreign tables specified in rels. + This function is called when is executed + on foreign tables. rels is the list of + Relation data structure that indicates + a foreign table to truncate. rels_extra the list of + int value, which delivers extra information about + a foreign table to truncate. Possible values are + TRUNCATE_REL_CONTEXT_NORMAL, which means that + the foreign table is specified WITHOUT ONLY clause + in TRUNCATE, + TRUNCATE_REL_CONTEXT_ONLY, which means that + the foreign table is specified WITH ONLY clause, + and TRUNCATE_REL_CONTEXT_CASCADING, + which means that the foreign table is not specified explicitly, + but will be truncated due to dependency (for example, partition table). + There is one-to-one mapping between rels and + rels_extra. The number of entries is the same + between the two lists. + + + + behavior defines how foreign tables should + be truncated, using as possible values DROP_RESTRICT, + which means that RESTRICT option is specified, + and DROP_CASCADE, which means that + CASCADE option is specified, in + TRUNCATE command. + + + + restart_seqs is set to true + if RESTART IDENTITY option is specified in + TRUNCATE command. It is false + if CONTINUE IDENTITY option is specified. + + + + TRUNCATE invokes + ExecForeignTruncate once per foreign server + that foreign tables to truncate belong to. This means that all foreign + tables included in rels must belong to the same + server. + + + + If the ExecForeignTruncate pointer is set to + NULL, attempts to truncate foreign tables will + fail with an error message. + + + FDW Routines for Row Locking diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index b0792a13b1..fd34956936 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -63,9 +63,10 @@ Now you need only SELECT from a foreign table to access the data stored in its underlying remote table. You can also modify - the remote table using INSERT, UPDATE, or - DELETE. (Of course, the remote user you have specified - in your user mapping must have privileges to do these things.) + the remote
Re: TRUNCATE on foreign table
2021年4月8日(木) 18:25 Fujii Masao : > > On 2021/04/08 15:48, Kohei KaiGai wrote: > > 2021年4月8日(木) 15:04 Fujii Masao : > >> > >> 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 is executed, only the contents in the > >> parent will be truncated. I was thinking that this behavior should be > >> applied to the foreign table 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 ONLY in 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 explicitly specify 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 should be 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 > discussion and 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
Re: TRUNCATE on foreign table
BLE tru_rtable0, tru_rtable1, tru_ptable, tru_ptable__p0, tru_pk_table,
tru_fk_table,
+tru_rtable_parent,tru_rtable_child, tru_rtable0_child;
+
-- ===
-- test IMPORT FOREIGN SCHEMA
-- ===
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 0f2397df49..46ab2e6708 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -1065,6 +1065,50 @@ EndDirectModify(ForeignScanState *node);
+
+FDW Routines for Truncate
+
+void
+ExecForeignTruncate(List *rels, List *rels_extra,
+DropBehavior behavior, bool restart_seqs);
+
+
+ Truncate a set of foreign tables defined by
+ rels belonging to the same foreign server.
+ This optional function is called during execution of
+ TRUNCATE for each foreign server involved
+ in one TRUNCATE command (note that invocations
+ are not per foreign table).
+
+ rels_extra delivers extra information about
+ the context where the foreign tables are truncated. It is a list of
integers and has same length with
+ rels. TRUNCATE_REL_CONTEXT_NORMAL
means that
+ the foreign table is specified WITHOUT ONLY clause,
and TRUNCATE_REL_CONTEXT_ONLY means
+ specified WITH ONLY clause.
TRUNCATE_REL_CONTEXT_CASCADING
+ value means that foreign tables are not specified in the
TRUNCATE,
+ but truncated due to dependency (for example, partition table).
+
+
+
+ If the ExecForeignTruncate pointer is set to
+ NULL, attempts to truncate the foreign table will
+ fail with an error message.
+
+
+
+ behavior defines how foreign tables should
+ be truncated, using as possible values DROP_RESTRICT
+ and DROP_CASCADE (to map with the equivalents of
+ TRUNCATE).
+
+
+
+ restart_seqs is set to true
+ if RESTART IDENTITY was supplied in the
+ TRUNCATE.
+
+
+
FDW Routines for Row Locking
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index b0792a13b1..fd34956936 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -63,9 +63,10 @@
Now you need only SELECT from a foreign table to access
the data stored in its underlying remote table. You can also modify
- the remote table using INSERT, UPDATE,
or
- DELETE. (Of course, the remote user you have specified
- in your user mapping must have privileges to do these things.)
+ the remote table using INSERT, UPDATE,
+ DELETE, or TRUNCATE.
+ (Of course, the remote user you have specified in your user mapping must
+ have privileges to do these things.)
@@ -436,6 +437,31 @@ OPTIONS (ADD password_required 'false');
+
+ Truncatability Options
+
+
+By default all foreign tables using postgres_fdw are
assumed
+to be truncatable. This may be overridden using the following option:
+
+
+
+
+
+ truncatable
+
+
+ This option controls whether postgres_fdw allows
+ foreign tables to be truncated using TRUNCATE
+ command. It can be specified for a foreign table or a foreign server.
+ A table-level option overrides a server-level option.
+ The default is true.
+
+
+
+
+
+
Importing Options
diff --git a/doc/src/sgml/ref/truncate.sgml b/doc/src/sgml/ref/truncate.sgml
index 91cdac5562..7899106ba1 100644
--- a/doc/src/sgml/ref/truncate.sgml
+++ b/doc/src/sgml/ref/truncate.sgml
@@ -172,9 +172,8 @@ TRUNCATE [ TABLE ] [ ONLY ] name [
- TRUNCATE 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 can be used for foreign tables if
+ the foreign data wrapper supports, for instance, see .
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 87f9bdaef0..7bad33bafb 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -59,6 +59,7 @@
#include "commands/typecmds.h"
#include "commands/user.h"
#include "executor/executor.h"
+#include "foreign/fdwapi.h"
#include "foreign/foreign.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
@@ -310,6 +311,21 @@ struct DropRelationCallbackState
#defineATT_FOREIGN_TABLE 0x0020
#defineATT_PARTITIONED_INDEX 0x0040
+/*
+ * ForeignTruncateInfo
+ *
+ * Information related to truncation of foreign tables. This is used for
+ * the elements in a hash table. It uses the server OID as lookup key,
+ * and includes a per-server list of all foreign tables involved in the
+ * truncation.
+ */
+typedef struct ForeignTruncateInfo
+{
+ Oid serverid;
+ List *r
Re: TRUNCATE on foreign table
On 2021/04/08 15:48, Kohei KaiGai wrote:
2021年4月8日(木) 15:04 Fujii Masao :
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 is executed, only the contents in the parent will be
truncated. I was thinking that this behavior should be applied to the foreign table
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 ONLY in 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 explicitly specify 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 should be 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
discussion and change the behavior later if necessary.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 6a61d83862..82aa14a65d 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -92,7 +92,6 @@ static PGconn *connect_pg_server(ForeignServer *server,
UserMapping *user);
static void disconnect_pg_server(ConnCacheEntry *entry);
static void check_conn_params(const char **keywords, const char **values,
UserMapping *user);
static void configure_remote_session(PGconn *conn);
-static void do_sql_command(PGconn *conn, const char *sql);
static void begin_remote_xact(ConnCacheEntry *entry);
static void pgfdw_xact_callback(XactEvent event, void *arg);
static void pgfdw_subxact_callback(SubXactEvent event,
@@ -568,7 +567,7 @@ configure_remote_session(PGconn *conn)
/*
* Convenience subroutine to issue a non-data-returning SQL command to remote
*/
-static void
+void
do_sql_command(PGconn *conn, const char *sql)
{
PGresult *res;
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 5aa3455e30..bdc4c3620d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -56,6 +56,7 @@
#include "utils/rel.h"
#include "utils/syscache.h"
#include "utils/typcache.h"
+#include "commands/tablecmds.h"
/*
* Global context for foreign_expr_walker's search of an expression tree.
@@ -2172,6 +2173,43 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List
**retrieved_attrs)
deparseRelation(buf, rel);
}
+/*
+ * Construct a simple "TRUNCATE rel" statement
+ */
+void
+deparseTruncateSql(StringInfo buf,
+ List *rels,
+ List *rels_extra,
+ DropBehavior behavior,
+ bool restart_seqs)
+{
+ ListCell *lc1,
+ *lc2;
+
+ appendStringInfoString(buf, "TRUNCATE ");
+
+ forboth(lc1, rels, lc2, rels_extra)
+ {
+ Relationrel = lfirst(lc1);
+ int extra = lfirst_int(lc2);
+
+ if (lc1 != list_head(rels))
+ appendStringInfoString(buf, ", ");
+ if (extra & TRUNCATE_REL_CONTEXT_ONLY)
+ appendStringInfoString(buf, "ONLY ");
+
+ deparseRelation(buf, rel);
+ }
+
+ appendStringInfo(b
Re: TRUNCATE on foreign table
2021年4月8日(木) 15:04 Fujii Masao : > > 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 is executed, only the contents in the parent > will be truncated. I was thinking that this behavior should be applied to the > foreign table 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 ONLY in 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 argue that 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 directly 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
Re: TRUNCATE on foreign table
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 is executed, only the contents in the parent will be truncated. I was thinking that this behavior should be applied to the foreign table 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 ONLY in TRUNCATE 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 argue that 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 directly specified 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
Re: TRUNCATE on foreign table
2021年4月8日(木) 11:44 Fujii Masao : > > On 2021/04/08 10:56, Kohei KaiGai wrote: > > 2021年4月8日(木) 4:19 Fujii Masao : > >> > >> 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_CASCADING is 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 seem 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 for that 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 "partition leafs" 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. > + TRUNCATE specifies the foreign tables with > + ONLY clause, remove queries over the > + postgres_fdw also specify remote tables with > + ONLY clause, that will truncate only parent > + portion of the remote table. In the results, it looks like > +
Re: TRUNCATE on foreign table
On 2021/04/08 10:56, Kohei KaiGai wrote: 2021年4月8日(木) 4:19 Fujii Masao : 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_CASCADING is 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 seem to 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 for that 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 "partition leafs" 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. + TRUNCATE specifies the foreign tables with + ONLY clause, remove queries over the + postgres_fdw also specify remote tables with + ONLY clause, that will truncate only parent + portion of the remote table. In the results, it looks like + TRUNCATE 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 still used 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 to 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 ONLY is specified or not) of the table found first is used. For example, when we execute "TRUNCATE ONLY tbl, tbl" and "TRUNCATE tbl, ONLY tbl", the former truncates only parent table because "ONLY tbl" is found first. But the latter truncates the parent
Re: TRUNCATE on foreign table
2021年4月8日(木) 4:19 Fujii Masao : > > 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_CASCADING is 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 that 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? 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 to 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. Best regards, Best regards, -- HeteroDB, Inc / The PG-Strom Project KaiGai Kohei
Re: TRUNCATE on foreign table
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_CASCADING is really required. With the patch, both inherited and referencing relations are marked as TRUNCATE_REL_CONTEXT_CASCADING? Is this ok for that use? 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 either of 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
Re: TRUNCATE on foreign table
> 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 : > > On Tue, Apr 6, 2021 at 10:15 PM Kazutaka Onishi 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 pgsql14-truncate-on-foreign-table.v16.patch Description: Binary data
Re: TRUNCATE on foreign table
On Tue, Apr 6, 2021 at 10:15 PM Kazutaka Onishi 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
Re: TRUNCATE on foreign table
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 : > > On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi 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 pgsql14-truncate-on-foreign-table.v15.patch Description: Binary data
Re: TRUNCATE on foreign table
On Tue, Apr 6, 2021 at 5:36 PM Kazutaka Onishi 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
Re: TRUNCATE on foreign table
On Tue, Apr 6, 2021 at 5:36 PM 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. 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
Re: TRUNCATE on foreign table
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 Truncatability Options, 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 : > > On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi 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 Truncatability Options, all > the documentation related to it be under this new section. > > By default all foreign tables using > postgres_fdw 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: > > > 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. >
Re: TRUNCATE on foreign table
On Mon, Apr 5, 2021 at 8:47 PM Kazutaka Onishi 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 Truncatability Options, all the documentation related to it be under this new section. By default all foreign tables using postgres_fdw 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: 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
Re: TRUNCATE on foreign table
> 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 : > > On Mon, Apr 5, 2021 at 7:38 PM Kazutaka Onishi 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
Re: TRUNCATE on foreign table
On Mon, Apr 5, 2021 at 7:38 PM Kazutaka Onishi 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
Re: TRUNCATE on foreign table
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 : > > On Sun, Apr 4, 2021 at 10:23 PM Kazutaka Onishi 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 =
Re: TRUNCATE on foreign table
On Sun, Apr 4, 2021 at 10:23 PM Kazutaka Onishi 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();
+deparseTruncateSql(, 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();
+deparseTruncateSql(, 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
Re: TRUNCATE on foreign table
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 <> -v > <> 6) to apply patch, git apply <>.patch thanks, I've removed these whitespaces and confirmed no warnings occurred when I run "git apply <>.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 : > > On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi 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 <> -v > <> 6) to apply patch, git apply <>.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 | > 1679091c5a
Re: TRUNCATE on foreign table
On Sun, Apr 4, 2021 at 12:00 PM Kazutaka Onishi 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 <> -v <> 6) to apply patch, git apply <>.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
Re: TRUNCATE on foreign table
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 : > > On Sun, Apr 4, 2021 at 12:48 PM Kazutaka Onishi 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 pgsql14-truncate-on-foreign-table.v11.patch Description: Binary data
Re: TRUNCATE on foreign table
On Sun, Apr 4, 2021 at 12:48 PM Kazutaka Onishi 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
Re: TRUNCATE on foreign table
2021年4月4日(日) 13:07 Bharath Rupireddy : > > On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu 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.gk2...@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
Re: TRUNCATE on foreign table
On Sat, Apr 3, 2021 at 8:31 PM Zhihong Yu 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
Re: TRUNCATE on foreign table
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, _oid, HASH_ENTER, ); and + while ((ft_info = hash_seq_search()) != NULL) +* Now go through the hash table, and process each entry associated to the +* 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 wrote: > Hi, > + TRUNCATE for each foreign server being involved > + in one TRUNCATE 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 '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 > 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 : >> > >> > 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 : >> > > >> > > >> > > >> > > 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 >> >
Re: TRUNCATE on foreign table
Hi, + TRUNCATE for each foreign server being involved + in one TRUNCATE 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 '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 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 : > > > > 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 : > > > > > > > > > > > > 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 >
Re: TRUNCATE on foreign table
On Sat, Apr 3, 2021 at 7:16 PM Kazutaka Onishi 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? +TRUNCATE can be used for foreign tables if the foreign data wrapper supports, for instance, see . With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: TRUNCATE on foreign table
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 : > > 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 : > > > > > > > > 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 pgsql14-truncate-on-foreign-table.v8.patch Description: Binary data
Re: TRUNCATE on foreign table
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 : > > > > 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 pgsql14-truncate-on-foreign-table.v7.patch Description: Binary data
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
2021年4月1日(木) 18:53 Fujii Masao : > > 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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
2021年3月30日(火) 2:53 Fujii Masao : > > 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
Re: TRUNCATE on foreign table
On 2021/03/30 10:11, Kohei KaiGai wrote: 2021年3月30日(火) 3:45 Fujii Masao : 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
Re: TRUNCATE on foreign table
2021年3月30日(火) 3:45 Fujii Masao : > > 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
Re: TRUNCATE on foreign table
2021年3月30日(火) 2:54 Fujii Masao : > > 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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
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
Re: TRUNCATE on foreign table
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 signature.asc Description: PGP signature
Re: TRUNCATE on foreign table
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
