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

On Tue, Dec 20, 2022 at 2:29 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2022-Oct-31, Peter Smith wrote:
>
> > 6. add_policy_clauses
> >
> > + else
> > + {
> > + append_bool_object(policyStmt, "present", false);
> > + }
> >
> > Something seems strange. Probably I'm wrong but just by code
> > inspection it looks like there is potential for there to be multiple
> > param {present:false} JSON objects:
> >
> > {"present" :false},
> > {"present" :false},
> > {"present" :false},
> >
> > Shouldn't those all be array elements or something? IIUC apart from
> > just DDL, the JSON idea was going to (in future) allow potential
> > machine manipulation of the values prior to the replication, but
> > having all these ambiguous-looking objects does not seem to lend
> > itself to that idea readily. How to know what are each of those params
> > representing?
>
> Do you mean that a single JSON object has multiple member with
> "present":"false"?  That sounds like something we should never produce,
> and post-processing to remove them does not sound good either.  Is that
> really what is happening, or do I misunderstand?
>

Yes, that is what I meant.

The add_policy_clauses code below is from latest patch v54-0001:

+static void
+add_policy_clauses(ObjTree *ret, Oid policyOid, List *roles, bool do_qual,
+    bool do_with_check)
+{
+ Relation polRel = table_open(PolicyRelationId, AccessShareLock);
+ HeapTuple polTup = get_catalog_object_by_oid(polRel,
Anum_pg_policy_oid, policyOid);
+ Form_pg_policy polForm;
+
+ if (!HeapTupleIsValid(polTup))
+ elog(ERROR, "cache lookup failed for policy with OID %u", policyOid);
+
+ polForm = (Form_pg_policy) GETSTRUCT(polTup);
+
+ /* Add the "ON table" clause */
+ append_object_object(ret, "ON %{table}D",
+ new_objtree_for_qualname_id(RelationRelationId,
+ polForm->polrelid));
+
+ /*
+ * Add the "TO role" clause, if any.  In the CREATE case, it always
+ * contains at least PUBLIC, but in the ALTER case it might be empty.
+ */
+ if (roles)
+ {
+ List    *list = NIL;
+ ListCell   *cell;
+
+ foreach(cell, roles)
+ {
+ RoleSpec   *spec = (RoleSpec *) lfirst(cell);
+
+ list = lappend(list,
+    new_object_object(new_objtree_for_rolespec(spec)));
+ }
+ append_array_object(ret, "TO %{role:, }R", list);
+ }
+ else
+ append_not_present(ret);
+
+ /* Add the USING clause, if any */
+ if (do_qual)
+ {
+ Datum deparsed;
+ Datum storedexpr;
+ bool isnull;
+
+ storedexpr = heap_getattr(polTup, Anum_pg_policy_polqual,
+   RelationGetDescr(polRel), &isnull);
+ if (isnull)
+ elog(ERROR, "null polqual expression in policy %u", policyOid);
+ deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
+ append_string_object(ret, "USING (%{expression}s)", "expression",
+ TextDatumGetCString(deparsed));
+ }
+ else
+ append_not_present(ret);
+
+ /* Add the WITH CHECK clause, if any */
+ if (do_with_check)
+ {
+ Datum deparsed;
+ Datum storedexpr;
+ bool isnull;
+
+ storedexpr = heap_getattr(polTup, Anum_pg_policy_polwithcheck,
+   RelationGetDescr(polRel), &isnull);
+ if (isnull)
+ elog(ERROR, "null polwithcheck expression in policy %u", policyOid);
+ deparsed = DirectFunctionCall2(pg_get_expr, storedexpr, polForm->polrelid);
+ append_string_object(ret, "WITH CHECK (%{expression}s)",
+ "expression", TextDatumGetCString(deparsed));
+ }
+ else
+ append_not_present(ret);
+
+ relation_close(polRel, AccessShareLock);
+}

Actually, I have not yet tried running this so maybe I am mistaken,
but looking at the code above I thought if 'roles' is NULL and
'do_qual' is false and 'do_with_check' is false then the logic could
end up doing:

+ append_not_present(ret);
+ append_not_present(ret);
+ append_not_present(ret);

> Obviously, if you have an object with several sub-objects, each of the
> sub-objects can have its own "present:false" label.  The idea is that
> the clause that each subobject represents may not be in the command as
> written by the user; but perhaps a post-processor of the JSON blob wants
> to change things so that the clause does appear in the final output.
> And this should be doable for each individual optional clause in each
> command, which means that, yeah, there should be multiple
> "present:false" pairs in a single JSON blob, in different paths.
>
> (For example, if the user writes "CREATE SEQUENCE foobar", we would get
> a tree that has {fmt: "CACHE %{value}", present: false, value: 32}, so
> if you just convert that to text DDL without further ado you would get
> the original command verbatim; but you can poke the "present" to true so
> you would get "CREATE SEQUENCE foobar CACHE 32".)
>
>
> Also, I think I came up with the idea of having "present:boolean" a bit
> late in the development of this code, so it's quite possible that there
> are commands that are inconsistent in their support of this pattern.
> That makes it especially important to review the representation of each
> command carefully.
>

------
Kind Regards,
Peter Smith.
Fujitsu Australia