Re: Support logical replication of DDLs

Alvaro Herrera <alvherre@alvh.no-ip.org>

From: Alvaro Herrera <alvherre@alvh.no-ip.org>

To: Ajin Cherian <itsajin@gmail.com>

Cc: Peter Smith <smithpb2250@gmail.com>, vignesh C <vignesh21@gmail.com>, Zheng Li <zhengli10@gmail.com>, li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2023-02-09T10:55:22Z

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 →

Add a run_as_owner option to subscriptions.
- 482675987bcd 16.0 cited
Refactor pgoutput_change().
- da324d6cd45b 16.0 cited
Print the correct aliases for DML target tables in ruleutils.
- df931e9ab35b 11.20 landed
- c8a5f1685fb7 15.3 landed
- 4efb4f0d4878 13.11 landed
- 3dd287c14fac 12.15 landed
- 393430f57544 16.0 landed
- 14345f3c6a7b 14.8 landed
Fix object identity string for transforms
- 9a312562314a 16.0 landed
Add grantable MAINTAIN privilege and pg_maintain role.
- 60684dd834a2 16.0 cited
Get rid of recursion-marker values in enum AlterTableType
- 840ff5f451cd 16.0 cited
Release cache tuple when no longer needed
- ed0fbc8e5ac9 15.0 cited
Empty search_path in logical replication apply worker and walsender.
- 11da97024abb 14.0 cited
Refactor format_type APIs to be more modular
- a26116c6cbf4 11.0 cited
Use wrappers of PG_DETOAST_DATUM_PACKED() more.
- 3a0d473192b2 10.0 cited

I happened to notice that MINVFUNC in 0003 displays like this
      "fmt": "MINVFUNC==%{type}T",
in some cases; this appears in the JSON that's emitted by the regression
tests at some point.  How can we detect this kind of thing, so that
these mistakes become self-evident?  I thought the intention of the
regress module was to run the deparsed code, so the syntax error should
have become obvious.

...

Oh, I see the problem.  There are two 'fmt' lines for that clause (and
many others), one of which is used when the clause is not present.  So
there's never a syntax error, because this one never expands other than
to empty.

AFAICS this defeats the purpose of the 'present' field.  I mean, if the
clause is never to deparse, then why have it there in the first place?
If we want to have it, then it has to be correct.


I think we should design the code to avoid the repetition, because that
has an inherent risk of typo bugs and such.  Maybe we should set forth
policy that each 'fmt' string should appear in the source code only
once.  So instead of this

+       /* MINVFUNC */
+       if (OidIsValid(agg->aggminvtransfn))
+               tmp = new_objtree_VA("MINVFUNC=%{type}T", 1,
+                                    "type", ObjTypeObject,
+                                    new_objtree_for_qualname_id(ProcedureRelationId,
+                                                                agg->aggminvtransfn));
+       else
+       {
+               tmp = new_objtree("MINVFUNC==%{type}T");
+               append_bool_object(tmp, "present", false);
+       }

we would have something like

   tmp = new_objtree("MINVFUNC=%{type}T");
   if (OidIsValid(agg->aggminvtransfn))
   {
      append_bool_object(tmp, "present", true);
      append...(tmp, "type", new_objtree_for_qualname_id(ProcedureRelationId, agg->aggminvtransfn));
   }
   else
   {
      append_bool_object(tmp, "present", false);
   }


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)