Re: Support logical replication of DDLs

On Sat, 15 Apr 2023 at 06:38, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, 14 Apr 2023 at 13:06, vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Few comments:

Few more comments:
1) since missing_ok is passed as false, if there is an error the error
will be handled in find_string_in_jsonbcontainer, "missing operator
name" handling can be removed from here:
+/*
+ * Expand a JSON value as an operator name. The value may contain element
+ * "schemaname" (optional).
+ */
+static void
+expand_jsonval_operator(StringInfo buf, JsonbValue *jsonval)
+{
+ char *str;
+ JsonbContainer *data = jsonval->val.binary.data;
+
+ str = find_string_in_jsonbcontainer(data, "schemaname", true, NULL);
+ /* Schema might be NULL or empty */
+ if (str != NULL && str[0] != '\0')
+ {
+ appendStringInfo(buf, "%s.", quote_identifier(str));
+ pfree(str);
+ }
+
+ str = find_string_in_jsonbcontainer(data, "objname", false, NULL);
+ if (!str)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("missing operator name"));
+

2) This should be present at the beginning of the file before the functions:
+#define ADVANCE_PARSE_POINTER(ptr,end_ptr) \
+ do { \
+ if (++(ptr) >= (end_ptr)) \
+ ereport(ERROR, \
+
errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
+ errmsg("unterminated format
specifier")); \
+ } while (0)
+

3) Should we add this to the documentation, we have documented other
event triggers like ddl_command_start, ddl_command_end, table_rewrite
and sql_drop at [1]:
+ runlist = EventTriggerCommonSetup(command->parsetree,
+
EVT_TableInitWrite,
+
"table_init_write",
+
&trigdata);

4) The inclusion of stringinfo.h is not required, I was able to
compile the code without it:
+ * src/backend/commands/ddl_json.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "lib/stringinfo.h"
+#include "tcop/ddl_deparse.h"
+#include "utils/builtins.h"
+#include "utils/jsonb.h"

5) schema and typmodstr might not be allocated, should we still call pfree?
+ schema = find_string_in_jsonbcontainer(data, "schemaname", true, NULL);
+ typename = find_string_in_jsonbcontainer(data, "typename", false, NULL);
+ typmodstr = find_string_in_jsonbcontainer(data, "typmod", true, NULL);
+ is_array = find_bool_in_jsonbcontainer(data, "typarray");
+ switch (is_array)
+ {
+ case tv_true:
+ array_decor = "[]";
+ break;
+
+ case tv_false:
+ array_decor = "";
+ break;
+
+ case tv_absent:
+ default:
+ ereport(ERROR,
+
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("missing typarray element"));
+ }
+
+ if (schema == NULL)
+ appendStringInfo(buf, "%s", quote_identifier(typename));
+ else if (schema[0] == '\0')
+ appendStringInfo(buf, "%s", typename); /* Special
typmod needs */
+ else
+ appendStringInfo(buf, "%s.%s", quote_identifier(schema),
+ quote_identifier(typename));
+
+ appendStringInfo(buf, "%s%s", typmodstr ? typmodstr : "", array_decor);
+ pfree(schema);
+ pfree(typename);
+ pfree(typmodstr);

6) SHould the following from ddl_deparse_expand_command function
header be moved to expand_one_jsonb_element function header, as the
specified are being handled in expand_one_jsonb_element.
* % expand to a literal %
* I expand as a single, non-qualified identifier
* D expand as a possibly-qualified identifier
* T expand as a type name
* O expand as an operator name
* L expand as a string literal (quote using single quotes)
* s expand as a simple string (no quoting)
* n expand as a simple number (no quoting)
* R expand as a role name (possibly quoted name, or PUBLIC)

7) In ddl_deparse.c we have used elog(ERROR, ...) for error handling
and in ddl_json.c we have used ereport(ERROR, ...) for error handling,
Is this required for any special handling?

8) Is this required as part of create table implementation:
8.a)
+/*
+ * EventTriggerAlterTypeStart
+ * Save data about a single part of an ALTER TYPE.
+ *
+ * ALTER TABLE can have multiple subcommands which might include DROP COLUMN
+ * command and ALTER TYPE referring the drop column in USING expression.
+ * As the dropped column cannot be accessed after the execution of DROP COLUMN,
+ * a special trigger is required to handle this case before the drop column is
+ * executed.
+ */
+void
+EventTriggerAlterTypeStart(AlterTableCmd *subcmd, Relation rel)
+{

8.b)
+/*
+ * EventTriggerAlterTypeEnd
+ * Finish up saving an ALTER TYPE command, and add it to
command list.
+ */
+void
+EventTriggerAlterTypeEnd(Node *subcmd, ObjectAddress address, bool rewrite)

9) Since we need only the table and index related implementation in
0001, the rest can be moved to a different patch accordingly:
+/*
+ * Return the given object type as a string.
+ *
+ * If isgrant is true, then this function is called while deparsing GRANT
+ * statement and some object names are replaced.
+ */
+const char *
+stringify_objtype(ObjectType objtype, bool isgrant)
+{
+ switch (objtype)
+ {
+ case OBJECT_AGGREGATE:
+ return "AGGREGATE";
+ case OBJECT_CAST:
+ return "CAST";
+ case OBJECT_COLLATION:
+ return "COLLATION";
+ case OBJECT_COLUMN:
+ return isgrant ? "TABLE" : "COLUMN";

10) json_trivalue should be added to typedefs:
+typedef enum
+{
+ tv_absent,
+ tv_true,
+ tv_false
+} json_trivalue;

[1] - https://www.postgresql.org/docs/current/event-trigger-definition.html

Regards,
Vignesh