On Mon, Apr 17, 2023 at 5:32 PM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > On Monday, April 10, 2023 7:20 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Fri, Apr 7, 2023 at 8:52 AM houzj.f...@fujitsu.com > > <houzj.f...@fujitsu.com> wrote: > > > > > > Sorry, there was a miss when rebasing the patch which could cause the > > > CFbot to fail and here is the correct patch set. > > > > > > > I see the following note in the patch: "Note: For ATTACH/DETACH PARTITION, > > we haven't added extra logic on the subscriber to handle the case where the > > table on the publisher is a PARTITIONED TABLE while the target table on the > > subscriber side is a NORMAL table. We will research this more and improve it > > later." and wonder what should we do about this. I can think of the > > following > > possibilities: (a) Convert a non-partitioned table to a partitioned one and > > then > > attach the partition; (b) Add the partition as a separate new table; (c) > > give an > > error that table types mismatch. For Detach partition, I don't see much > > possibility than giving an error that no such partition exists or something > > like > > that. Even for the Attach operation, I prefer (c) as the other options > > don't seem > > logical to me and may add more complexity to this work. > > > > Thoughts? > > I also think option (c) makes sense and is same as the latest patch's > behavior. > > Attach the new version patch set which include the following changes: >
Few comments for ddl_deparse.c in patch dated April17: 1) append_format_string() I think we need to have 'Assert(sub_fmt)' here like we have it in all other similar functions (append_bool_object, append_object_object, ...) 2) append_object_to_format_string() here we have code piece : if (sub_fmt == NULL || tree->fmtinfo == NULL) return sub_fmt; but sub_fmt will never be null when we reach this function as all its callers assert for null sub_fmt. So that means when tree->fmtinfo is null, we end up returning sub_fmt as it is, instead of extracting object name from that. Is this intended? 3) We can remove extra spaces after full-stop in the comment below /* * Deparse a ColumnDef node within a typed table creation. This is simpler * than the regular case, because we don't have to emit the type declaration, * collation, or default. Here we only return something if the column is being * declared NOT NULL. ... deparse_ColumnDef_typed() 4) These functions are not being used, do we need to retain these in this patch? deparse_Type_Storage() deparse_Type_Receive() deparse_Type_Send() deparse_Type_Typmod_In() deparse_Type_Typmod_Out() deparse_Type_Analyze() deparse_Type_Subscript() 5) deparse_AlterRelation() We have below variable initialized to false in the beginning 'bool istype = false;' And then we have many conditional codes using the above, eg: istype ? "ATTRIBUTE" : "COLUMN". We are not changing 'istype' anywhere and it is hard-coded in the beginning. It means there are parts of code in this function which will never be htt (written for 'istype=true' case) , so why do we need this variable and conditional code around it? 6) There are plenty of places where we use 'append_not_present' without using 'append_null_object'. Do we need to have 'append_null_object' along with 'append_not_present' at these places? 7) deparse_utility_command(): Rather than inject --> Rather than injecting thanks Shveta