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)
> + {
> + Relation rel = 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 <pryz...@telsasoft.com>
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);
</programlisting>
- Truncate a set of foreign tables specified in <literal>rels</literal>.
+ Truncate foreign tables.
This function is called when <xref linkend="sql-truncate"/> is executed
- on foreign tables. <literal>rels</literal> is the list of
- <structname>Relation</structname> data structure that indicates
- a foreign table to truncate. <literal>rels_extra</literal> the list of
- <literal>int</literal> value, which delivers extra information about
- a foreign table to truncate. Possible values are
+ on a foreign table. <literal>rels</literal> is a list of
+ <structname>Relation</structname> data structures for the
+ foreign tables to be truncated. <literal>rels_extra</literal> is a list of
+ corresponding <literal>int</literal> values which provide extra information about
+ the foreign tables. Each element of rels_extra may have the value
<literal>TRUNCATE_REL_CONTEXT_NORMAL</literal>, which means that
- the foreign table is specified WITHOUT <literal>ONLY</literal> clause
+ the foreign table is specified <emphasis>WITHOUT</emphasis> the <literal>ONLY</literal> clause
in <command>TRUNCATE</command>,
<literal>TRUNCATE_REL_CONTEXT_ONLY</literal>, which means that
- the foreign table is specified WITH <literal>ONLY</literal> clause,
- and <literal>TRUNCATE_REL_CONTEXT_CASCADING</literal>,
+ the foreign table is specified <emphasis>WITH</emphasis> the <literal>ONLY</literal> clause,
+ or <literal>TRUNCATE_REL_CONTEXT_CASCADING</literal>,
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 <literal>rels</literal> and
- <literal>rels_extra</literal>. The number of entries is the same
- between the two lists.
+ but will be truncated due to a dependency (for example, a table partition).
</para>
<para>
- <literal>behavior</literal> defines how foreign tables should
- be truncated, using as possible values <literal>DROP_RESTRICT</literal>,
- which means that <literal>RESTRICT</literal> option is specified,
- and <literal>DROP_CASCADE</literal>, which means that
- <literal>CASCADE</literal> option is specified, in
+ <literal>behavior</literal> must be specified as
+ <literal>DROP_RESTRICT</literal> or <literal>DROP_CASCADE</literal>.
+ If specified as <literal>DROP_RESTRICT</literal>, the
+ <literal>RESTRICT</literal> option will be included in the
<command>TRUNCATE</command> command.
+ If specified as <literal>DROP_CASCADE</literal>, the
+ <literal>CASCADE</literal> option will be included.
</para>
<para>
- <literal>restart_seqs</literal> is set to <literal>true</literal>
- if <literal>RESTART IDENTITY</literal> option is specified in
- <command>TRUNCATE</command> command. It is <literal>false</literal>
- if <literal>CONTINUE IDENTITY</literal> option is specified.
+ If <literal>restart_seqs</literal> is specified as <literal>true</literal>,
+ the <command>TRUNCATE</command> command will include the
+ <literal>RESTART IDENTITY</literal> option.
+ Otherwise, the <command>TRUNCATE</command> command will include the
+ <literal>CONTINUE IDENTITY</literal> option.
</para>
<para>
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');
<listitem>
<para>
This option controls whether <filename>postgres_fdw</filename> allows
- foreign tables to be truncated using <command>TRUNCATE</command>
+ foreign tables to be truncated using the <command>TRUNCATE</command>
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 <literal>true</literal>.
diff --git a/doc/src/sgml/ref/truncate.sgml b/doc/src/sgml/ref/truncate.sgml
index acf3633be4..9d846f88c9 100644
--- a/doc/src/sgml/ref/truncate.sgml
+++ b/doc/src/sgml/ref/truncate.sgml
@@ -173,7 +173,7 @@ TRUNCATE [ TABLE ] [ ONLY ] <replaceable class="parameter">name</replaceable> [
<para>
<command>TRUNCATE</command> can be used for foreign tables if
- the foreign data wrapper supports, for instance,
+ supported by the foreign data wrapper, for instance,
see <xref linkend="postgres-fdw"/>.
</para>
</refsect1>
--
2.17.0
>From 513bb389e3e3c3cfe47518164c6ce49736854756 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 10 Apr 2021 22:53:21 -0500
Subject: [PATCH 2/3] Convert an if/else if without an else to a switch
---
contrib/postgres_fdw/deparse.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index bdc4c3620d..2c702c0e37 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2204,10 +2204,15 @@ deparseTruncateSql(StringInfo buf,
appendStringInfo(buf, " %s IDENTITY",
restart_seqs ? "RESTART" : "CONTINUE");
- if (behavior == DROP_RESTRICT)
+ switch (behavior)
+ {
+ case DROP_RESTRICT:
appendStringInfoString(buf, " RESTRICT");
- else if (behavior == DROP_CASCADE)
+ break;
+ case DROP_CASCADE:
appendStringInfoString(buf, " CASCADE");
+ break;
+ }
}
/*
--
2.17.0
>From 76d783f4e472ea466cca0d126798505f851e54d0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Sat, 10 Apr 2021 23:08:58 -0500
Subject: [PATCH 3/3] Test integer using == and not &
---
contrib/postgres_fdw/deparse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 2c702c0e37..c2b1269f73 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -2195,7 +2195,7 @@ deparseTruncateSql(StringInfo buf,
if (lc1 != list_head(rels))
appendStringInfoString(buf, ", ");
- if (extra & TRUNCATE_REL_CONTEXT_ONLY)
+ if (extra == TRUNCATE_REL_CONTEXT_ONLY)
appendStringInfoString(buf, "ONLY ");
deparseRelation(buf, rel);
--
2.17.0
