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

On Mon, Apr 17, 2023 18:32 PM vignesh C <vignesh21@gmail.com> wrote:
> 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:

Thanks for your comments.
Improved the patch set according to your comments. Please refer to the details below.

===
About the comments in [1]:

> 1) I felt is_present_flag variable can be removed by moving 
> "object_name = append_object_to_format_string(tree, sub_fmt);" inside 
> the if condition:
> +static void
> +append_bool_object(ObjTree *tree, char *sub_fmt, bool value) {
> +       ObjElem    *param;
> +       char       *object_name = sub_fmt;
> +       bool            is_present_flag = false;
> +
> +       Assert(sub_fmt);
> +
> +       /*
> +        * Check if the format string is 'present' and if yes, store the boolean
> +        * value
> +        */
> +       if (strcmp(sub_fmt, "present") == 0)
> +       {
> +               is_present_flag = true;
> +               tree->present = value;
> +       }
> +
> +       if (!is_present_flag)
> +               object_name = append_object_to_format_string(tree, 
> + sub_fmt);
> +
> +       param = new_object(ObjTypeBool, object_name);
> +       param->value.boolean = value;
> +       append_premade_object(tree, param); }
> 
> By changing it to something like below:
> +static void
> +append_bool_object(ObjTree *tree, char *sub_fmt, bool value) {
> +       ObjElem    *param;
> +       char       *object_name = sub_fmt;
> +
> +       Assert(sub_fmt);
> +
> +       /*
> +        * Check if the format string is 'present' and if yes, store the boolean
> +        * value
> +        */
> +       if (strcmp(sub_fmt, "present") == 0)
> +       {
> +               tree->present = value;
> +               object_name = append_object_to_format_string(tree, sub_fmt);
> +       }
> +
> +       param = new_object(ObjTypeBool, object_name);
> +       param->value.boolean = value;
> +       append_premade_object(tree, param); }

Changed.

> 2) We could remove the temporary variable tmp_str here:
> +       if (start_ptr != NULL && end_ptr != NULL)
> +       {
> +               length = end_ptr - start_ptr - 1;
> +               tmp_str = (char *) palloc(length + 1);
> +               strncpy(tmp_str, start_ptr + 1, length);
> +               tmp_str[length] = '\0';
> +               appendStringInfoString(&object_name, tmp_str);
> +               pfree(tmp_str);
> +       }
> 
> by changing to:
> +       if (start_ptr != NULL && end_ptr != NULL)
> +               appendBinaryStringInfo(&object_name, start_ptr + 1,
> end_ptr - start_ptr - 1);

Changed.

> 3) I did not see the usage of ObjTypeFloat type used anywhere, we 
> could remove it:
> +typedef enum
> +{
> +       ObjTypeNull,
> +       ObjTypeBool,
> +       ObjTypeString,
> +       ObjTypeArray,
> +       ObjTypeInteger,
> +       ObjTypeFloat,
> +       ObjTypeObject
> +} ObjType;

Removed.

> 4) I noticed that none of the file names in src/backend/commands uses 
> "_" in the filenames, but in case of ddl_deparse.c and ddl_json.c we 
> have used "_", it might be better to be consistent with other 
> filenames in this directory:
> 
> diff --git a/src/backend/commands/Makefile 
> b/src/backend/commands/Makefile index 48f7348f91..171dfb2800 100644
> --- a/src/backend/commands/Makefile
> +++ b/src/backend/commands/Makefile
> @@ -29,6 +29,8 @@ OBJS = \
>         copyto.o \
>         createas.o \
>         dbcommands.o \
> +       ddl_deparse.o \
> +       ddl_json.o \
>         define.o \
>         discard.o \
>         dropcmds.o \

Changed.

> 5) The following includes are no more required in ddl_deparse.c as we 
> have removed support for deparsing of other objects:
> #include "catalog/pg_am.h"
> #include "catalog/pg_aggregate.h"
> #include "catalog/pg_authid.h"
> #include "catalog/pg_cast.h"
> #include "catalog/pg_conversion.h"
> #include "catalog/pg_depend.h"
> #include "catalog/pg_extension.h"
> #include "catalog/pg_foreign_data_wrapper.h"
> #include "catalog/pg_foreign_server.h"
> #include "catalog/pg_language.h"
> #include "catalog/pg_largeobject.h"
> #include "catalog/pg_opclass.h"
> #include "catalog/pg_operator.h"
> #include "catalog/pg_opfamily.h"
> #include "catalog/pg_policy.h"
> #include "catalog/pg_range.h"
> #include "catalog/pg_rewrite.h"
> #include "catalog/pg_sequence.h"
> #include "catalog/pg_statistic_ext.h"
> #include "catalog/pg_transform.h"
> #include "catalog/pg_ts_config.h"
> #include "catalog/pg_ts_dict.h"
> #include "catalog/pg_ts_parser.h"
> #include "catalog/pg_ts_template.h"
> #include "catalog/pg_user_mapping.h"
> #include "foreign/foreign.h"
> #include "mb/pg_wchar.h"
> #include "nodes/nodeFuncs.h"
> #include "nodes/parsenodes.h"
> #include "parser/parse_type.h"

Removed.

===
About the comments in [2]:

> 1) We could add a space after the 2nd parameter
> + * Note we don't have the luxury of sprintf-like compiler warnings 
> +for
> + * malformed argument lists.
> + */
> +static ObjTree *
> +new_objtree_VA(char *fmt, int numobjs,...)

Changed.

> 2) I felt objtree_to_jsonb_element is a helper function for 
> objtree_to_jsonb_rec, we should update the comments accordingly:
> +/*
> + * Helper for objtree_to_jsonb: process an individual element from an 
> +object or
> + * an array into the output parse state.
> + */
> +static void
> +objtree_to_jsonb_element(JsonbParseState *state, ObjElem *object,
> +                                                JsonbIteratorToken 
> +elem_token) {
> +       JsonbValue      val;
> +
> +       switch (object->objtype)
> +       {
> +               case ObjTypeNull:
> +                       val.type = jbvNull;
> +                       pushJsonbValue(&state, elem_token, &val);
> +                       break;

Changed.

> 3) domainId parameter change should be removed from the first patch:
> +static List *
> +obtainConstraints(List *elements, Oid relationId, Oid domainId,
> +                                 ConstraintObjType objType) {
> +       Relation        conRel;
> +       ScanKeyData key;
> +       SysScanDesc scan;
> +       HeapTuple       tuple;
> +       ObjTree    *constr;
> +       Oid                     relid;
> +
> +       /* Only one may be valid */
> +       Assert(OidIsValid(relationId) ^ OidIsValid(domainId));
> +
> +       relid = OidIsValid(relationId) ? ConstraintRelidTypidNameIndexId :
> +                       ConstraintTypidIndexId;

Removed in the last version.

> 4) Do we have any scenario for CONSTRAINT_TRIGGER in table/index, if 
> so could we add a test for this?
> +                       case CONSTRAINT_UNIQUE:
> +                               contype = "unique";
> +                               break;
> +                       case CONSTRAINT_TRIGGER:
> +                               contype = "trigger";
> +                               break;
> +                       case CONSTRAINT_EXCLUSION:
> +                               contype = "exclusion";
> +                               break;

No. Moved from 0001 patch to later patch (the deparser for the rest commands).

> 5) The below code adds information about compression but the comment 
> says "USING clause", the comment should be updated accordingly:
> +       /* USING clause */
> +       tmp_obj = new_objtree("COMPRESSION");
> +       if (coldef->compression)
> +               append_string_object(tmp_obj, 
> + "%{compression_method}I",
> +
> "compression_method", coldef->compression);
> +       else
> +       {
> +               append_null_object(tmp_obj, "%{compression_method}I");
> +               append_not_present(tmp_obj);
> +       }
> +       append_object_object(ret, "%{compression}s", tmp_obj);

Changed.

> 6) Generally we add append_null_object followed by append_not_present, 
> but it is not present for "COLLATE" handling, is this correct?
> +       tmp_obj = new_objtree("COMPRESSION");
> +       if (coldef->compression)
> +               append_string_object(tmp_obj, 
> + "%{compression_method}I",
> +
> "compression_method", coldef->compression);
> +       else
> +       {
> +               append_null_object(tmp_obj, "%{compression_method}I");
> +               append_not_present(tmp_obj);
> +       }
> +       append_object_object(ret, "%{compression}s", tmp_obj);
> +
> +       tmp_obj = new_objtree("COLLATE");
> +       if (OidIsValid(typcollation))
> +               append_object_object(tmp_obj, "%{name}D",
> +
> new_objtree_for_qualname_id(CollationRelationId,
> +
>                                           typcollation));
> +       else
> +               append_not_present(tmp_obj);
> +       append_object_object(ret, "%{collation}s", tmp_obj);

Changed.

> 7) I felt attrTup can be released after get_atttypetypmodcoll as we 
> are not using the tuple after that, I'm not sure if it is required to 
> hold the reference to this tuple till the end of the function:
> +static ObjTree *
> +deparse_ColumnDef_typed(Relation relation, List *dpcontext, ColumnDef
> *coldef)
> +{
> +       ObjTree    *ret = NULL;
> +       ObjTree    *tmp_obj;
> +       Oid                     relid = RelationGetRelid(relation);
> +       HeapTuple       attrTup;
> +       Form_pg_attribute attrForm;
> +       Oid                     typid;
> +       int32           typmod;
> +       Oid                     typcollation;
> +       bool            saw_notnull;
> +       ListCell   *cell;
> +
> +       attrTup = SearchSysCacheAttName(relid, coldef->colname);
> +       if (!HeapTupleIsValid(attrTup))
> +               elog(ERROR, "could not find cache entry for column
> \"%s\" of relation %u",
> +                        coldef->colname, relid);
> +       attrForm = (Form_pg_attribute) GETSTRUCT(attrTup);
> +
> +       get_atttypetypmodcoll(relid, attrForm->attnum,
> +                                                 &typid, &typmod,
> &typcollation);
> +
> +       /*
> +        * Search for a NOT NULL declaration. As in deparse_ColumnDef,
> we rely on
> +        * finding a constraint on the column rather than coldef->is_not_null.
> +        * (This routine is never used for ALTER cases.)
> +        */
> +       saw_notnull = false;
> +       foreach(cell, coldef->constraints)
> +       {
> +               Constraint *constr = (Constraint *) lfirst(cell);
> +
> +               if (constr->contype == CONSTR_NOTNULL)
> +               {
> +                       saw_notnull = true;
> +                       break;
> +               }
> +       }

'attrTup' can not be released earlier as it is used till the end in the form of 'attrForm'.

> 8) This looks like ALTER TABLE ... SET/RESET, the function header 
> should be updated accordingly:
> /*
>  * ... ALTER COLUMN ... SET/RESET (...)
>  *
>  * Verbose syntax
>  * RESET|SET (%{options:, }s)
>  */
> static ObjTree *
> deparse_RelSetOptions(AlterTableCmd *subcmd) {
> List    *sets = NIL;
> ListCell   *cell;
> bool is_reset = subcmd->subtype == AT_ResetRelOptions;

Changed.

> 9) Since we don't replicate temporary tables, is this required:
> +/*
> + * Deparse the ON COMMIT ... clause for CREATE ... TEMPORARY ...
> + *
> + * Verbose syntax
> + * ON COMMIT %{on_commit_value}s
> + */
> +static ObjTree *
> +deparse_OnCommitClause(OnCommitAction option) {
> +       ObjTree    *ret  = new_objtree("ON COMMIT");
> +       switch (option)

I will consider this in the next version.

> 10) Since we don't support MATERIALIZED VIEW, VIEW and FOREIGN TABLE, 
> they can be removed:
> +       switch (rel->rd_rel->relkind)
> +       {
> +               case RELKIND_RELATION:
> +               case RELKIND_PARTITIONED_TABLE:
> +                       reltype = "TABLE";
> +                       break;
> +               case RELKIND_INDEX:
> +               case RELKIND_PARTITIONED_INDEX:
> +                       reltype = "INDEX";
> +                       break;
> +               case RELKIND_VIEW:
> +                       reltype = "VIEW";
> +                       break;
> +               case RELKIND_COMPOSITE_TYPE:
> +                       reltype = "TYPE";
> +                       istype = true;
> +                       break;
> +               case RELKIND_FOREIGN_TABLE:
> +                       reltype = "FOREIGN TABLE";
> +                       break;
> +               case RELKIND_MATVIEW:
> +                       reltype = "MATERIALIZED VIEW";
> +                       break;

The code is changed in a way that these are no-op and return NULL instead of returning a valid value. I think this
shouldsuffice.
 
Please let me know if you think these must be removed.

===
About the comments in [3]:

> 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"));
> +

Removed.

> 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)
> +

Changed.

> 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);

Added.

> 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"

Removed.

> 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);

Changed code to do NULL initialization in the begining and call pfree only if these are allocated.

> 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)

It seems more suitable here as this is the exposed function
and thus complete details here make sense. But I have changed comment little
bit to indicate that actual conversion work happens in expand_one_jsonb_element().
Please let me know if you still think it is better to move the comments.

>  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?

I checked occurences of elog in ddl_deparse.c. That looked appropriate to me
as they are internal errors which we usually don't expect user to encounter.
In fact I changed few of ereport added earlier by me to elog to be consistent.
The occurences of ereport in ddl_json.c  also looks fine to me. 
Please let me know if you want any specific error handling to be changed.

> 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)

It is used to store the using expression of alter column type command,
and the using expression(usingexpr) is used in the deparser. So I think it
cannot be removed/moved to other patches.

> 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";

Changed.

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

Added.

Attach the new patch set and thanks Shveta for helping
address the above comments.

Apart from above comments.
The new version patch also did the following changes:

- Fixed the cfbot warnings about the header file.
- Added documents for ALTER TABLE rewrite restrictions.
- Fixed the crash when applying create index concurrently.
- Used the consistent API as DML apply when switch the current role
 to the another before applying.

[1] - https://www.postgresql.org/message-id/CALDaNm1aTHyeMfmkyunq%3DHZ6dyOJNqgszhmsLkeVMEgWfJ8frA%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CALDaNm1RnYRfzsL4GfznU4zuoPMkgnAAM8Ons3kCtLr2Tdzoow%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/CALDaNm1NeyTrCDizXHvUqhbOdH2%3D%2Bf%3DR8aX3r0AbDr7rRJeQAA%40mail.gmail.com

Best Regards,
Hou zj