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-20T10:10:44Z
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 9:11 AM shveta malik <shveta.malik@gmail.com> wrote:
>
>
> Few comments for ddl_deparse.c in patch dated April17:
>
> 1) append_format_string()
> I think we need to have 'Assert(sub_fmt)' here like we have it in all
> other similar functions (append_bool_object, append_object_object,
> ...)
>

+1.

> 2) append_object_to_format_string()
> here we have code piece :
>         if (sub_fmt == NULL || tree->fmtinfo == NULL)
>                 return sub_fmt;
> but sub_fmt will never be null when we reach this function as all its
> callers assert for null sub_fmt. So that means when tree->fmtinfo is
> null, we end up returning sub_fmt as it is, instead of extracting
> object name from that. Is this intended?
>
> 3) We can remove extra spaces after full-stop in the comment below
> /*
>  * Deparse a ColumnDef node within a typed table creation. This is simpler
>  * than the regular case, because we don't have to emit the type declaration,
>  * collation, or default.  Here we only return something if the column is being
>  * declared NOT NULL.
>  ...
>  deparse_ColumnDef_typed()
>

Which full-stop you are referring to here first or second? I see there
is a tab after the first one which should be changed to space.

>
> 4) These functions are not being used, do we need to retain these in this patch?
> deparse_Type_Storage()
> deparse_Type_Receive()
> deparse_Type_Send()
> deparse_Type_Typmod_In()
> deparse_Type_Typmod_Out()
> deparse_Type_Analyze()
> deparse_Type_Subscript()
>

I don't think we need to retain these. And, it seems they are already removed.

> 5) deparse_AlterRelation()
> We have below variable initialized to false in the beginning
> 'bool            istype = false;'
> And then we have many conditional codes using the above, eg: istype ?
> "ATTRIBUTE" : "COLUMN".  We are not changing 'istype' anywhere and it
> is hard-coded in the beginning. It means there are parts of code in
> this function which will never be htt (written for 'istype=true' case)
> , so why do we need this variable and conditional code around it?
>

I don't see any usage of istype. We should simply remove the use of
istype and use "COLUMN" directly.

>
> 6) There are plenty of places where we use 'append_not_present'
> without using 'append_null_object'.
> Do we need to have 'append_null_object' along with
> 'append_not_present' at these places?
>

What is the difference if we add a null object or not before not_present?

-- 
With Regards,
Amit Kapila.