On Tue, Dec 20, 2022 at 2:29 AM Alvaro Herrera <alvhe...@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