On Feb 8, 2025 at 12:55 +0800, Sami Imseih <samims...@gmail.com>, wrote: > > Fix CI failure of doc build in v1 patch. > > > > Thanks for the patch! I am +1 for this, but I have a few comments: Hi, tanks for review. > > > > 1/ In the IDENTITY case, the remote side may not be > > able to handle the DEFAULT value. Yes, and done. > > 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. Done. > > > > 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. 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. > > > > 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. Done.
Patch V2 addressed the comments. [0] https://www.postgresql.org/message-id/6cecef0e-ee14-473c-bb0a-6aa61f539a66%40Spark -- Zhang Mingli HashData
v2-0001-CREATE-FOREIGN-TABLE-LIKE.patch
Description: Binary data
