Add a test for "cannot truncate foreign table"

Started by Yugo NAGATAover 3 years ago10 messages
#1Yugo NAGATA
nagata@sraoss.co.jp
1 attachment(s)

Hello,

During checking regression tests of TRUNCATE on foreign
tables for other patch [1]/messages/by-id/20220527172543.0a2fdb469cf048b81c0967d3@sraoss.co.jp, I found that there is no test
for foreign tables that don't support TRUNCATE.

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

I attached a patch.

[1]: /messages/by-id/20220527172543.0a2fdb469cf048b81c0967d3@sraoss.co.jp

--
Yugo NAGATA <nagata@sraoss.co.jp>

Attachments:

test_cannot_truncate_foreigin_table.patchtext/x-diff; name=test_cannot_truncate_foreigin_table.patchDownload
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 0029f36b35..261af1a8b5 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -246,6 +246,8 @@ UPDATE agg_csv SET a = 1;
 ERROR:  cannot update foreign table "agg_csv"
 DELETE FROM agg_csv WHERE a = 100;
 ERROR:  cannot delete from foreign table "agg_csv"
+TRUNCATE agg_csv;
+ERROR:  cannot truncate foreign table "agg_csv"
 -- but this should be allowed
 SELECT * FROM agg_csv FOR UPDATE;
   a  |    b    
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index 563d824ccc..46670397ca 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -166,6 +166,7 @@ SELECT tableoid::regclass, b FROM agg_csv;
 INSERT INTO agg_csv VALUES(1,2.0);
 UPDATE agg_csv SET a = 1;
 DELETE FROM agg_csv WHERE a = 100;
+TRUNCATE agg_csv;
 -- but this should be allowed
 SELECT * FROM agg_csv FOR UPDATE;
 
#2Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Yugo NAGATA (#1)
Re: Add a test for "cannot truncate foreign table"

On 2022/06/30 10:48, Yugo NAGATA wrote:

Hello,

During checking regression tests of TRUNCATE on foreign
tables for other patch [1], I found that there is no test
for foreign tables that don't support TRUNCATE.

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

I attached a patch.

Thanks for the patch! It looks good to me.
I changed the status of this patch to ready-for-committer,
and will commit it barring any objeciton.

Regards,

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

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Fujii Masao (#2)
Re: Add a test for "cannot truncate foreign table"

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2022/06/30 10:48, Yugo NAGATA wrote:

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

Thanks for the patch! It looks good to me.
I changed the status of this patch to ready-for-committer,
and will commit it barring any objeciton.

This seems like a fairly pointless expenditure of test cycles
to me. Perhaps more importantly, what will you do when
somebody adds truncate support to that FDW? I don't think
there's an inherent reason for it to be read-only.

regards, tom lane

#4Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Tom Lane (#3)
Re: Add a test for "cannot truncate foreign table"

On 2022/07/08 0:33, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2022/06/30 10:48, Yugo NAGATA wrote:

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

Thanks for the patch! It looks good to me.
I changed the status of this patch to ready-for-committer,
and will commit it barring any objeciton.

This seems like a fairly pointless expenditure of test cycles
to me. Perhaps more importantly, what will you do when
somebody adds truncate support to that FDW?

One idea is to create dummy FDW (like foreign_data.sql regression test does) not supporting TRUNCATE and use it for the test.

BTW, file_fdw already has the similar test cases for INSERT, UPDATE and DELETE, as follows.

-- updates aren't supported
INSERT INTO agg_csv VALUES(1,2.0);
ERROR: cannot insert into foreign table "agg_csv"
UPDATE agg_csv SET a = 1;
ERROR: cannot update foreign table "agg_csv"
DELETE FROM agg_csv WHERE a = 100;
ERROR: cannot delete from foreign table "agg_csv"

Regards,

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

#5Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Fujii Masao (#4)
Re: Add a test for "cannot truncate foreign table"

At Fri, 8 Jul 2022 01:06:18 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2022/07/08 0:33, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2022/06/30 10:48, Yugo NAGATA wrote:

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

Thanks for the patch! It looks good to me.
I changed the status of this patch to ready-for-committer,
and will commit it barring any objeciton.

This seems like a fairly pointless expenditure of test cycles
to me. Perhaps more importantly, what will you do when
somebody adds truncate support to that FDW?

As Fujii-san mentioned below, file_fdw has tests for INSERT/UPDATE and
DELETE. If somebody added DELETE to file_fdw, the test for DELETE
rejection would be turned into a normal test of the DELETE function.
I don't see a difference between TRUNCATE and other updating commands
from this point of view.

One idea is to create dummy FDW (like foreign_data.sql regression test
does) not supporting TRUNCATE and use it for the test.

I think the proposed test is not that for FDW framework, but for a
specific FDW module, file_fdw.

BTW, file_fdw already has the similar test cases for INSERT, UPDATE
and DELETE, as follows.

-- updates aren't supported
INSERT INTO agg_csv VALUES(1,2.0);
ERROR: cannot insert into foreign table "agg_csv"
UPDATE agg_csv SET a = 1;
ERROR: cannot update foreign table "agg_csv"
DELETE FROM agg_csv WHERE a = 100;
ERROR: cannot delete from foreign table "agg_csv"

Agreed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#6Yugo NAGATA
nagata@sraoss.co.jp
In reply to: Kyotaro Horiguchi (#5)
Re: Add a test for "cannot truncate foreign table"

On Fri, 08 Jul 2022 09:44:10 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Fri, 8 Jul 2022 01:06:18 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2022/07/08 0:33, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2022/06/30 10:48, Yugo NAGATA wrote:

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

Thanks for the patch! It looks good to me.
I changed the status of this patch to ready-for-committer,
and will commit it barring any objeciton.

This seems like a fairly pointless expenditure of test cycles
to me. Perhaps more importantly, what will you do when
somebody adds truncate support to that FDW?

As Fujii-san mentioned below, file_fdw has tests for INSERT/UPDATE and
DELETE. If somebody added DELETE to file_fdw, the test for DELETE
rejection would be turned into a normal test of the DELETE function.
I don't see a difference between TRUNCATE and other updating commands
from this point of view.

One idea is to create dummy FDW (like foreign_data.sql regression test
does) not supporting TRUNCATE and use it for the test.

I think the proposed test is not that for FDW framework, but for a
specific FDW module, file_fdw.

Yes, the patch is an improvement for the test of flie_fdw.

If we want to test foreign table modifications for the FDW framework,
we will have to add such tests in foreign_data.sql, because foreign
table modifications are tested only for postgres_fdw and file_fdw.

BTW, file_fdw already has the similar test cases for INSERT, UPDATE
and DELETE, as follows.

-- updates aren't supported
INSERT INTO agg_csv VALUES(1,2.0);
ERROR: cannot insert into foreign table "agg_csv"
UPDATE agg_csv SET a = 1;
ERROR: cannot update foreign table "agg_csv"
DELETE FROM agg_csv WHERE a = 100;
ERROR: cannot delete from foreign table "agg_csv"

Agreed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata@sraoss.co.jp>

#7Etsuro Fujita
etsuro.fujita@gmail.com
In reply to: Yugo NAGATA (#6)
Re: Add a test for "cannot truncate foreign table"

On Fri, Jul 8, 2022 at 11:07 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote:

On Fri, 08 Jul 2022 09:44:10 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:

At Fri, 8 Jul 2022 01:06:18 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2022/07/08 0:33, Tom Lane wrote:

On 2022/06/30 10:48, Yugo NAGATA wrote:

When a foreign table has handler but doesn't support TRUNCATE,
an error "cannot truncate foreign table xxx" occurs. So, what
about adding a test this message output? We can add this test
for file_fdw because it is one of the such foreign data wrappers.

This seems like a fairly pointless expenditure of test cycles
to me. Perhaps more importantly, what will you do when
somebody adds truncate support to that FDW?

As Fujii-san mentioned below, file_fdw has tests for INSERT/UPDATE and
DELETE. If somebody added DELETE to file_fdw, the test for DELETE
rejection would be turned into a normal test of the DELETE function.
I don't see a difference between TRUNCATE and other updating commands
from this point of view.

I agree on this point.

One idea is to create dummy FDW (like foreign_data.sql regression test
does) not supporting TRUNCATE and use it for the test.

If we want to test foreign table modifications for the FDW framework,
we will have to add such tests in foreign_data.sql, because foreign
table modifications are tested only for postgres_fdw and file_fdw.

Rather than doing so, I'd vote for adding a test case to file_fdw, as
proposed in the patch, because that would be much simpler and much
less expensive.

Best regards,
Etsuro Fujita

#8Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Etsuro Fujita (#7)
Re: Add a test for "cannot truncate foreign table"

On 2022/07/08 17:03, Etsuro Fujita wrote:

Rather than doing so, I'd vote for adding a test case to file_fdw, as
proposed in the patch, because that would be much simpler and much
less expensive.

So ISTM that most agreed to push Nagata-san's patch adding the test for TRUNCATE on foreign table with file_fdw. So barring any objection, I will commit the patch.

Regards,

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

#9Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Fujii Masao (#8)
Re: Add a test for "cannot truncate foreign table"

On 2022/07/15 16:52, Fujii Masao wrote:

On 2022/07/08 17:03, Etsuro Fujita wrote:

Rather than doing so, I'd vote for adding a test case to file_fdw, as
proposed in the patch, because that would be much simpler and much
less expensive.

So ISTM that most agreed to push Nagata-san's patch adding the test for TRUNCATE on foreign table with file_fdw. So barring any objection, I will commit the patch.

Pushed. Thanks!

Regards,

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

#10Yugo NAGATA
nagata@sraoss.co.jp
In reply to: Fujii Masao (#9)
Re: Add a test for "cannot truncate foreign table"

On Wed, 20 Jul 2022 09:38:17 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2022/07/15 16:52, Fujii Masao wrote:

On 2022/07/08 17:03, Etsuro Fujita wrote:

Rather than doing so, I'd vote for adding a test case to file_fdw, as
proposed in the patch, because that would be much simpler and much
less expensive.

So ISTM that most agreed to push Nagata-san's patch adding the test for TRUNCATE on foreign table with file_fdw. So barring any objection, I will commit the patch.

Pushed. Thanks!

Thanks!

Regards,

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

--
Yugo NAGATA <nagata@sraoss.co.jp>