On Friday, July 8, 2022 10:26 AM Peter Smith <smithpb2250@gmail.com> wrote:
> Here are some review comments for the patch v11-0001:
Thanks for the comments.
> 4. src/backend/commands/ddl_deparse.c - <general>
>
> Lots of places are making calls to the new_objtree_VA function but some of them are a bit messy. I think the >
wrappingof the args to that function needs to be revisited and made consistent indentation everywhere to make them >
alleasier to read. IMO it is easier when the number of arg-groups is clear and each arg-group is on a new line.
> Like this example:
>
> column = new_objtree_VA("%{name}I WITH OPTIONS %{default}s %{not_null}s",
> 2,
> "type", ObjTypeString, "column",
> "name", ObjTypeString,
> coldef->colname);
I think both your suggestion and the following style are fine to me.
new_objtree_VA(fmt, num,
xxobj1,
xxobj2 );
So, I only changed the other style function calls.
> 30. src/backend/commands/ddl_deparse.c - deparse_RenameStmt
>
> + switch (node->renameType)
> + {
> + case OBJECT_SCHEMA:
> + {
> + renameStmt =
> + new_objtree_VA("ALTER SCHEMA %{identity}I RENAME TO %{newname}I",
> + 0);
> + append_string_object(renameStmt, "identity", node->subname); } break;
> + default:
> + elog(ERROR, "unsupported object type %d", node->renameType); }
> The switch with single case seems a bit overkill here. Wouldn’t just "if" be
> more appropriate?
I think it was intended for other RENAME deparsing which haven't been added but
will be added to the patch later. So, I didn't change this.
> 31. src/backend/commands/ddl_deparse.c - deparse_CreateSeqStmt
>
> This function looked very similar to the other function deparse_ColumnIdentity.
> Is it worth trying to combine these or have one of them just delegate to the
> other to reduce the cut/paste code?
Since they are used to deparse different commands, one for (column identity)
another for (create sequence), so I think the current style is fine.
> 40. src/backend/commands/ddl_json.c - <general>
>
> Many (but not all) of these comments (particularly the function header
> comments) seem to have double blank spaces in them after periods. I don’t
> think it is normal. Please remove the > extra spaces
I think this style is fine as I can see them in many other existing comments.
> 44.
> + if (value == NULL)
> + {
> + if (missing_ok)
> + return NULL;
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("missing element \"%s\" in json object", keyname))); }
>
> For some reason, it seems more intuitive IMO to handle the error case first here. YMMV.
Not sure about this.
> 62. src/include/catalog/pg_proc.dat
>
> +{ oid => '4642', descr => 'ddl deparse',
> + proname => 'ddl_deparse_to_json', prorettype => 'text',
> + proargtypes => 'pg_ddl_command', prosrc => 'ddl_deparse_to_json' }, {
> +oid => '4643', descr => 'json to string',
> + proname => 'ddl_deparse_expand_command', prorettype => 'text',
> + proargtypes => 'text', prosrc => 'ddl_deparse_expand_command' },
>
> My 1st impressions was the name "ddl_deparse_expand_command" does not see to reflext the description very well...
>
> Maybe calling it something like "ddl_json_to_command" is more accurate ?
I think "expand command" is close to the usage of this function so didn't change the
name for now. But I adjust the description here.
> 63. src/include/utils/builtins.h
>
> @@ -118,6 +118,7 @@ extern char *format_type_extended(Oid type_oid,
> int32 typemod, bits16 flags);
> extern char *format_type_be(Oid type_oid); extern char *format_type_be_qualified(Oid type_oid); extern char >
*format_type_with_typemod(Oidtype_oid, int32 typemod);
> +extern char *printTypmod(const char *typname, int32 typmod, Oid
> +typmodout);
>
> Notice that every of the format_type function name looks like format_type_XXX.
> Not that you have change the printTypmod to be extern then I woinder should the
> name also be changed (e.g. format_type_printmod) to have the consistent
> function naming.
I am not sure about this as the current name looks fine to me.
I agreed with the other comments and addressed them on the new version patch.
Attach the V12 patch set which include the following changes:
* Address comments from peter[1]
* Refactor the deparser and provide an option to control whether output the "not present" syntax part.
And for DDL replication, we don't WAL log the "not present" syntax string for now.
* Address most comments from Vignesh[2] except the one about pg_dump
vs the event trigger for ddl replication which need some more research.
[1] https://www.postgresql.org/message-id/CAHut%2BPs9QyGw4KRFP50vRnB0tJKbB_TS1E7rZ_-%2Bpc2Nvwv_zw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm2nFPMxUo%3D0zRUUA-v3_eRwRY%2Bii5nnG_PU%2B6jT7ta9dA%40mail.gmail.com
Best regards,
Hou zj
