On Thu, Mar 16, 2023 10:44 AM Wang, Wei/王 威 <wangw.fnst@fujitsu.com> wrote:
> On Tues, Mar 14, 2023 12:17 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > On Mon, Mar 13, 2023 at 2:24 AM Zheng Li <zhengli10@gmail.com> wrote:
> > >
> > > Thanks for working on the test coverage for CREATE and ALTER TABLE.
> > > I've made fixes for some of the failures in the v79 patch set
> > > (0002,
> > > 0003 and 0004 are updated). The changes includes:
> > > 1. Fixed a syntax error caused by ON COMMIT clause placement in
> > > deparse_CreateStmt.
> > > 2. Fixed deparse_Seq_As and start using it in
> > > deparse_CreateSeqStmt, this issue is also reported in [1].
> > > 3. Fixed a bug in append_not_present: the 'present: false' element
> > > can't be omitted even in non-verbose mode. It will cause syntax
> > > error on reformed command if 'present: false' element is missing
> > > but the fmt string indicates the corresponding object must be present.
> > > 4. Replaced if_not_exists with if_exists in deparse of
> > > AT_DropConstraint and AT_DropColumn.
> > > 5. Added missing CASCADE clause for AT_DropConstraint deparse.
> > > 6. Enabled the fixed test cases.
> > >
> >
> > I found out that the option ONLY was not parsed in the "CREATE INDEX"
> > command,
> > for eg: CREATE UNIQUE INDEX ... ON ONLY table_name ...
> >
> > I've fixed this in patch 0002.
>
> Thanks for the new patch set.
>
> Here are some comments:
>
> For v-80-0002* patch.
> 1. The comments atop the function deparse_IndexStmt.
> + * Verbose syntax
> + * CREATE %{unique}s INDEX %{concurrently}s %{if_not_exists}s
> +%{name}I ON
> + * %{table}D USING %{index_am}s %{definition}s %{with}s
> +%{tablespace}s
> + * %{where_clause}s %{nulls_not_distinct}s */ static ObjTree *
> +deparse_IndexStmt(Oid objectId, Node *parsetree)
>
> Since we added decoding for the [ONLY] option in this version, it
> seems that we also need to add related comments, like this:
> ```
> %{table}D USING %{index_am}s %{definition}s %{with}s %{tablespace}s
> ->
> %{only}s %{table}D USING %{index_am}s %{definition}s %{with}s
> %{tablespace}s ```
Added.
BTW, the parsed %{nulls_not_distinct}s is stored in the wrong order, so moved it from the last position before
%{with}s.
> ===
>
> For v-80-0003* patch.
> 2. In the function deparse_CreateTrigStmt.
> I think we need to parse the [OR REPLACE] option for CREATE TRIGGER command.
>
> And I think there are two similar missing in the functions
> deparse_DefineStmt_Aggregate (option [OR REPLACE]) and
> deparse_DefineStmt_Collation (option [IF NOT EXISTS]).
Added.
> ===
>
> For v-80-0004* patch.
> 3. There are some whitespace errors:
> Applying: Introduce the test_ddl_deparse_regress test module.
> .git/rebase-apply/patch:163: new blank line at EOF.
> +
> .git/rebase-apply/patch:3020: new blank line at EOF.
> +
> .git/rebase-apply/patch:4114: new blank line at EOF.
> +
> warning: 3 lines add whitespace errors.
Fixed.
Attach the new patch set which addressed above comments.
0002,0003,0004 patch has been updated in this version.
Best Regards,
Hou zj
