Subject: Re: Proposal to CREATE FOREIGN TABLE LIKE

From: Zhang Mingli <[email-masked]>
Date: Thu, 6 Feb 2025 18:09:42 +0800

On Feb 3, 2025 at 08:29 +0800, Michael Paquier <[email-masked]>, wrote:
> On Mon, Feb 03, 2025 at 06:22:13AM +0800, Mingli Zhang wrote:
> > Yes, I would like to provide a patch.
> >
> > Glad to see we have come to an agreement on this.
>
> Just adding my +1 here. FWIW.
Hi,

Patch added.

Added support for CREATE FOREIGN TABLE LIKE to enable the creation of foreign tables based on the column definitions, constraints of existing source tables.
This feature mirrors the behavior of CREATE TABLE LIKE, but ignores inapplicable options such as INCLUDING INDEXES and INCLUDING COMPRESSION for foreign tables.

I have disallowed the COMPRESSION option due to existing inconsistencies between STORAGE and COMPRESSION.
I’ve posted a summary of these issues at Inconsistency between Compression and Storage for Foreign Tables[0].

Once we align the behavior of STORAGE and COMPRESSION, I believe that COMPRESSION should also be copied for foreign tables, similar to STORAGE.
This might be beneficial for foreign data wrappers (FDWs) that take it into account.

[0] https://www.postgresql.org/message-id/6cecef0e-ee14-473c-bb0a-6aa61f539a66%40Spark

--
Zhang Mingli
HashData

---

From: Zhang Mingli <[email-masked]>
Date: Fri, 7 Feb 2025 22:24:36 +0800

> > Just adding my +1 here. FWIW.

Add it to commitfest: https://commitfest.postgresql.org/52/5557

--
Zhang Mingli
HashData

---

From: Zhang Mingli <[email-masked]>
Date: Sat, 8 Feb 2025 00:50:47 +0800

> Add it to commitfest: https://commitfest.postgresql.org/52/5557

Fix CI failure of doc build in v1 patch.

--
Zhang Mingli
HashData

---

From: Sami Imseih <[email-masked]>
Date: Fri, 7 Feb 2025 22:55:23 -0600

> Fix CI failure of doc build in v1 patch.

Thanks for the patch! I am +1 for this, but I have a few comments:

1/ In the IDENTITY case, the remote side may not be
able to handle the DEFAULT value. See the example below:

-- on the foreign server
postgres=# CREATE TABLE t2 (id int, c1 text);
CREATE TABLE

-- on the local server
postgres=#
postgres=# CREATE TABLE t1 (id int GENERATED ALWAYS AS IDENTITY, c1 text);
CREATE TABLE
postgres=# CREATE FOREIGN TABLE t2 (LIKE t1 INCLUDING INDEXES) server r1;
CREATE FOREIGN TABLE
postgres=# INSERT INTO t2 (c1) VALUES ('A');
INSERT 0 1
postgres=# SELECT * FROM t2;
 id | c1
----+----
    | A
(1 row)

This is also the reason foreign tables don't document IDENTITY as valid [1].
It may even be a bug for it to be allowed with the CREATE FOREIGN TABLE
syntax:

postgres=# CREATE FOREIGN TABLE t3 (id int GENERATED ALWAYS AS
IDENTITY, c1 text) server r1;
CREATE FOREIGN TABLE
postgres=# \d+ t3
                                                  Foreign table "public.t3"
 Column |  Type   | Collation | Nullable |           Default
 | FDW options | Storage  | Stats target | Description
--------+---------+-----------+----------+------------------------------+-------------+----------+--------------+-------------
 id     | integer |           | not null | generated always as
identity |             | plain    |              |
 c1     | text    |           |          |
 |             | extended |              |
Not-null constraints:
    "t3_id_not_null" NOT NULL "id"
Server: r1

2/  As IDENTITY, STORAGE is also allowed on foreign tables, which
does not make much sense either, as the fdw may not be Postgres,
or if it is Postgres, it may have a different STORAGE setting. This is
also not documented. I am inclined to say to not include it either.

I think we should not allow IDENTITY and STORAGE in this patch
as they are not documented [1] as is, and perhaps a separate discussion
to correct the behavior for the CREATE FOREIGN TABLE case.

3/ a minor nit on the comments.

instead of

+      Foreign tables have no real storage in PostgreSQL.

Can it just be

Foreign tables have no storage in PostgreSQL.

the "real" is not needed.

[1] https://www.postgresql.org/docs/current/sql-createforeigntable.html

--
Regards,

Sami

---

From: Zhang Mingli <[email-masked]>
Date: Sun, 9 Feb 2025 21:16:53 +0800

> > Thanks for the patch! I am +1 for this, but I have a few comments:
Hi, tanks for review.
> > able to handle the DEFAULT value.
Yes, and done.
> > also not documented. I am inclined to say to not include it either.
Done.
> > to correct the behavior for the CREATE FOREIGN TABLE case.
Yes, however, I found we have doc to tell users and they actually could ALTER FOREIGN TABLE to SET STORAGE...
There are several inconsistencies, I have a summary in Inconsistency between Compression and Storage for Foreign Tables[0]
We could discuss and fix them there.
> > the "real" is not needed.
Done.

Patch V2 addressed the comments.

--
Zhang Mingli
HashData

---

From: Sami Imseih <[email-masked]>
Date: Mon, 10 Feb 2025 18:14:14 -0600

> Patch V2 addressed the comments.

Overall this LGTM.

I still see a "no real storage" in v2 that should be removed
from the documentation.

+ Foreign tables have no real storage in PostgreSQL.
+ Inapplicable options: <literal>INCLUDING INDEXES</literal>,
<literal>INCLUDING STORAGE</literal>,

I think the test coverage to check for the negative conditions only is
enough.

Regards,

Sami

---

From: Zhang Mingli <[email-masked]>
Date: Tue, 11 Feb 2025 22:49:24 +0800

> > <literal>INCLUDING STORAGE</literal>,
Oh, I corrected another one in the code comments, but I forgot about this one.
Done in patch v3.
> I think the test coverage to check for the negative conditions only is

Hmm... I copied from the cases in the same file for each option.
There's no harm in having more tests, how about we keep them?

--
Zhang Mingli
HashData

---

From: Sami Imseih <[email-masked]>
Date: Tue, 11 Feb 2025 10:07:48 -0600

> Oh, I corrected another one in the code comments, but I forgot about this one.

I attached v4 with some slight modifications to the wording, otherwise
this looks good.

> There's no harm in having more tests, how about we keep them?

I agree. I was just saying the test cases you provided are
enough. No changes needed for the tests.

I have no further comments.

Regards,

Sami

---

From: Michael Paquier <[email-masked]>
Date: Mon, 17 Feb 2025 16:23:56 +0900

> I have no further comments.

The checks you are adding in the parse analysis of the LIKE clauses is
surprisingly light.

+ * For foreign tables, they have no storage in Postgres.
+ * Inapplicable options are ignored.

Wording is a bit strange here.

+ *	CREATE_TABLE_LIKE_COMPRESSION
+ *	CREATE_TABLE_LIKE_IDENTITY
+ *	CREATE_TABLE_LIKE_STORAGE
+ *	CREATE_TABLE_LIKE_INDEXES
[...]
+DROP FOREIGN TABLE ctl_foreign_table1;
+DROP FOREIGN TABLE ctl_foreign_table2;
+DROP FOREIGN TABLE ctl_foreign_table3;
+DROP FOREIGN TABLE ctl_foreign_table4;
+DROP FOREIGN TABLE ctl_foreign_table5;
+DROP FOREIGN TABLE ctl_foreign_table6;

What's the point of creating that many tables, one for each of the
four INCLUDING options you are testing?  It is possible to stack all
of them in a single CREATE TABLE command, still is that really
necessary if we have coverage with INCLUDING ALL and EXCLUDING ALL
(perhaps write it directly in the CREATE query rather than assume that
it is the default) as these are all-or-nothing switches for all the
option values.  Of course let's be careful with HIDE_TOAST_COMPRESSION
if need be.

Perhaps the original table should have a primary key, also to track
the fact that the NOT NULL constraint is always copied but its related
index is not?  Identity columns assign a NOT NULL constraint, as
documented these are always copied.  Just wanted to point out that
this works the same way for your implementation with foreign tables, 
so perhaps we should have a test for that.

Glad to see that you did not forget about statistics.  I didn't recall
that these were OK for foreign tables, TBH.

expandTableLikeClause() depends on the "constr" built in the first
phase of transformTableLikeClause() to bypass the parts related to
generated, default and constraints properties.  This dependency
between both routines should be documented, I guess.  Hmm..  Not sure
where.
--
Michael

---

From: Zhang Mingli <[email-masked]>
Date: Mon, 17 Feb 2025 19:14:59 +0800

> Wording is a bit strange here.
Hi, is this better?

 * Foreign tables do not store data in Postgres.

> option values. Of course let's be careful with HIDE_TOAST_COMPRESSION

I usually follow the cases within the same file; for each independent option, it becomes easier to identify which options are valid or invalid.
However, if you believe consolidating them into one is better, I’m fine with that. Updated.
> so perhaps we should have a test for that.
Done, although there is already one in column d.
> that these were OK for foreign tables, TBH.
I also didn't realize this until I wrote this patch. This could be useful for the planner？

> between both routines should be documented, I guess. Hmm.. Not sure
Comments are addressed except for this one, it might be worth considering another patch.

--
Zhang Mingli
HashData

---

From: Michael Paquier <[email-masked]>
Date: Tue, 18 Feb 2025 09:49:22 +0900

>  * Any options that are not applicable for foreign tables will be ignored:

I would do something like that, perhaps, though I could get that
people don't like this suggestion:
"Some options are ignored.  For example, as foreign tables have no
storage, these options have no effect: storage, compression, identity
and indexes.  Similarly, INCLUDING INDEXES is ignored from a view."

> I usually follow the cases within the same file; for each
> independent option, it becomes easier to identify which options are
> valid or invalid.  However, if you believe consolidating them into
> one is better, I’m fine with that. Updated.

It does not matter much in this case, IMO, what matters is to make
sure that the four additional checks you are adding in the
post-parsing paths are correctly ignored.  Passing down an INCLUDING
ALL does that just fine as you double-check the state of the table
with a \d+ meta-command.

> I also didn't realize this until I wrote this patch. This could be

Constraints can be used as hints in the planner when working on
foreign tables.  I'm pretty sure that this is the same reason here,
seeing that this is supported since v10 where statistics have been
introduced.  I would need to dig more into the code, but that's not
really the point for this thread..

> considering another patch.

Yeah, not entirely sure.  It does not prevent your patch to work, and
it is already documented that the "expand" part is done as-is because
it depends on the "transform".  So perhaps it's just OK like this.  I
withdraw my comment.

+      Inapplicable options: <literal>INCLUDING INDEXES</literal>, <literal>INCLUDING STORAGE</literal>,
+      <literal>INCLUDING COMPRESSION</literal>, <literal>INCLUDING IDENTITY</literal> are ignored.

I would remove this paragraph, actually.  The options supported are
listed by your patch, and that would be one area less to update if a
new INCLUDING flavor is added.

Copy-pasting the details of how the LIKE options work to the
create_foreign_table.sgml page is OK for me, and perhaps this will
diverge a bit from the CREATE TABLE part.  One thing is that LIKE is
not part of the SQL specification for CREATE FOREIGN TABLE.  Perhaps
this should be mentioned at the bottom of the page under the
"compatibility" section?
--
Michael