Re: Support logical replication of DDLs - Mailing list pgsql-hackers

On Sat, 15 Apr 2023 at 06:38, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 14 Apr 2023 at 13:06, vignesh C <vignesh21@gmail.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