On Mon, Mar 6, 2023 18:17 PM Wang, Wei/王 威 <wangw.fnst@fujitsu.com> wrote:
> On Mon, Mar 6, 2023 14:34 AM Ajin Cherian <itsajin@gmail.com> wrote:
> > Changes are in patch 1 and patch 2
>
> Thanks for updating the patch set.
>
> Here are some comments:
Thanks for your comments.
> For v-75-0002* patch.
> 1. In the function deparse_AlterRelation.
> + if ((sub->address.objectId != relId &&
> + sub->address.objectId != InvalidOid) &&
> + !(subcmd->subtype == AT_AddConstraint &&
> + subcmd->recurse) &&
> + istable)
> + continue;
>
> I think when we execute the command "ALTER TABLE ... ADD (index)"
> (subtype is AT_AddIndexConstraint or AT_AddIndex), this command will
> be skipped for parsing.
> I think we need to parse both types of commands here.
Fixed.
> ~~~
>
> 3. In the function deparse_CreateTrigStmt
> + tgargs = DatumGetByteaP(fastgetattr(trigTup,
> +
> Anum_pg_trigger_tgargs,
> +
> RelationGetDescr(pg_trigger),
> +
> &isnull));
> + if (isnull)
> + elog(ERROR, "null tgargs for trigger \"%s\"",
> + NameStr(trigForm->tgname));
> + argstr = (char *) VARDATA(tgargs);
> + lentgargs = VARSIZE_ANY_EXHDR(tgargs);
> a.
> I think it might be better to invoke the function DatumGetByteaP after
> checking the flag "isnull". Because if "isnull" is true, I think an
> unexpected pointer
> ((NULL)->va_header) will be used when invoking macro VARATT_IS_4B_U.
Fixed.
> b.
> Since commit 3a0d473 recommends using macro DatumGetByteaPP instead of
> DatumGetByteaP, and the function pg_get_triggerdef_worker also uses
> macro DatumGetByteaPP, I think it might be better to use DatumGetByteaPP here.
Changed.
> ~~~
>
> 4. In the function deparse_CreateTrigStmt
> + append_object_object(ret, "EXECUTE PROCEDURE %{function}s",
> tmp_obj);
>
> Since the use of the keyword "PROCEDURE" is historical, I think it
> might be better to use "FUNCTION".
Changed.
> ===
> For v-75-0004* patch.
> 5. File src/test/modules/test_ddl_deparse_regress/README.md
> +1. Test that the generated JSON blob is expected using SQL tests.
> +2. Test that the re-formed DDL command is expected using SQL tests.
> +3. Test that the re-formed DDL command has the same effect as the original
> command
> + by comparing the results of pg_dump, using the SQL tests in 1
> and 2.
> +4. Test that new DDL syntax is handled by the DDL deparser by capturing and
> deparing
> + DDL commands ran by pg_regress.
>
> Inconsistent spacing:
> \t -> blank space
Changed.
Attach the new patch set which addressed above comments and comments from [1].
0001,0002,0003,0004 patch has been updated in this version.
[1] -
https://www.postgresql.org/message-id/OS3PR01MB62752246D2A718126825A1809EB59%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Best Regards,
Hou zj
