On Thu, Apr 20, 2023 at 9:11 AM shveta malik <shveta.ma...@gmail.com> wrote: > > > 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, > ...) >
+1. > 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() > Which full-stop you are referring to here first or second? I see there is a tab after the first one which should be changed to space. > > 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() > I don't think we need to retain these. And, it seems they are already removed. > 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? > I don't see any usage of istype. We should simply remove the use of istype and use "COLUMN" directly. > > 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? > What is the difference if we add a null object or not before not_present? -- With Regards, Amit Kapila.