On Mon, Feb 17, 2025 at 07:14:59PM +0800, Zhang Mingli wrote: > On Feb 17, 2025 at 15:24 +0800, Michael Paquier <mich...@paquier.xyz>, wrote: >> + * For foreign tables, they have no storage in Postgres. >> + * Inapplicable options are ignored. >> >> Wording is a bit strange here. > > * Foreign tables do not store data in Postgres. > * 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 > useful for the planner？ 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.. >> 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. > > Comments are addressed except for this one, it might be worth > 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
signature.asc
Description: PGP signature
