Horton

Re: Support logical replication of DDLs

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, vignesh C <vignesh21@gmail.com>, Ajin Cherian <itsajin@gmail.com>, "Wei Wang (Fujitsu)" <wangw.fnst@fujitsu.com>, Runqi Tian <runqidev@gmail.com>, Peter Smith <smithpb2250@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Zheng Li <zhengli10@gmail.com>
Date: 2023-04-21T05:57:58Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add a run_as_owner option to subscriptions.

  2. Refactor pgoutput_change().

  3. Print the correct aliases for DML target tables in ruleutils.

  4. Fix object identity string for transforms

  5. Add grantable MAINTAIN privilege and pg_maintain role.

  6. Get rid of recursion-marker values in enum AlterTableType

  7. Release cache tuple when no longer needed

  8. Empty search_path in logical replication apply worker and walsender.

  9. Refactor format_type APIs to be more modular

  10. Use wrappers of PG_DETOAST_DATUM_PACKED() more.

On Thu, Apr 20, 2023 at 2:28 PM shveta malik <shveta.malik@gmail.com> wrote:
>
>  Few comments for ddl_json.c in the patch dated April17:
>
...
>
> 3) expand_jsonb_array()
> arrayelem is allocated as below, but not freed.
> initStringInfo(&arrayelem)
>
> 4) expand_jsonb_array(),
> we initialize iterator as below which internally does palloc
> it = JsonbIteratorInit(container);
> Shall this be freed at the end? I see usage of this function in other files. At a few places, it is freed while not freed at other places.
>

Normally, it is a good idea to free whenever the allocation is done in
a long-lived context. However, in some places, we free just for the
sake of cleanliness. I think we don't need to bother doing retail free
in this case unless it is allocated in some long-lived context.


> 5) deparse_ddl_json_to_string(char *json_str, char** owner)
> str = palloc(value->val.string.len + 1);
> we do  palloc here and return allocated memory to caller as 'owner'. Caller sets this 'owner' using SetConfigOption() which internally allocates new memory and copies 'owner' to that. So the memory allocated in deparse_ddl_json_to_string is never freed. Better way should be the caller passing this allocated memory to deparse_ddl_json_to_string() and freeing it when done. Thoughts?
>

I think that will complicate the code. I don't see the need to
allocate this in any longer-lived context, so we shouldn't be bothered
to retail pfree it.

> 6)expand_fmt_recursive():
> value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);
> Should this 'value' be freed at the end like we do at all other places in this file?
>

Yeah, we can do this for the sake of consistency.

Few comments on 0001 patch:
=============================
1.
+ case 'O':
+ specifier = SpecOperatorName;
+ break;
...
+ case 'R':
+ specifier = SpecRole;
+ break;
+ default:

Both of these specifiers don't seem to be used in the patch. So, we
can remove them. I see some references to 'O' in the comments, those
also need to be modified.

2.
+ /* For identity column, find the sequence owned by column in order
+ * to deparse the column definition */

In multi-line comments, the first and last lines should be empty.
Refer to multi-line comments at other places.

3.
+ return object_name.data;
+
+}

An extra empty line before } is not required.

-- 
With Regards,
Amit Kapila.