On Monday, July 10, 2023 3:22 AM Zheng Li <zhengl...@gmail.com> wrote: > > On Tue, Jun 27, 2023 at 6:16 AM vignesh C <vignes...@gmail.com> wrote: > > > While development, below are some of the challenges we faced: > > > 1. Almost all the members of the AlterTableType enum will have to be > annotated. > > 2. Complex functionalities which require access to catalog tables > > cannot be auto generated, custom functions should be written in this > > case. > > 3. Some commands might have completely custom code(no auto > generation) > > and in the alter/drop table case we will have hybrid implementation > > both auto generated and custom implementation. > > Thanks for providing the PoC for auto generation of the deparser code! > > I think this is the main difference between the deparser code and outfuncs.c. > There is no need for catalog access in outfuncs.c, which makes code generation > simpler for outfuncs.c and harder for the deparser. The hybrid implementation > of the deparser doesn't seem to make it more maintainable, it's probably more > confusing. Is it possible to automate the code with catalog access? There may > be common patterns in it also.
I think it's not great to automate the catalog access because of the following points: 1. Only annotating fields to access the catalog won't be sufficient, we need to tell the catalog's field, operator, etc., and writing such functions for access will vary based on the type of DDL command[1] and will increase the maintenance burden as well. Additionally, we may call some extra functions to get the required output. See RelationGetPartitionBound. 2. For part of the DDL creation, we need to access the information from catalog indirectly, for example when deparsing the CREATE TABLE command, the persistence/of_type/relkind need to be fetched from the Relation structure(get from relation_open()), so autogenerating the catalog access code won't be sufficient here. 3. Most of the catalog access common codes have already been compressed into common functions(new_jsonb_for_qualname_id/insert_collate_object) and is easy to maintain. IMO, automate these codes again doesn't improve the situation too much. 4. Apart from the common functions mentioned in 3. There are only a few cases where we need to access catalog directly in the deparser, so there is little common code can be automated. I think only the function calls like the following[2] can be automated, but the main deparsing logic need custom implementation. [1] deparse_Seq_OwnedBy() ... depRel = table_open(DependRelationId, AccessShareLock); ScanKeyInit(&keys[0], Anum_pg_depend_classid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(RelationRelationId)); ScanKeyInit(&keys[1], Anum_pg_depend_objid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(sequenceId)); ScanKeyInit(&keys[2], Anum_pg_depend_objsubid, BTEqualStrategyNumber, F_INT4EQ, Int32GetDatum(0)); scan = systable_beginscan(depRel, DependDependerIndexId, true, NULL, 3, keys); while (HeapTupleIsValid(tuple = systable_getnext(scan))) deparse_Constraints() ... conRel = table_open(ConstraintRelationId, AccessShareLock); ScanKeyInit(&key, Anum_pg_constraint_conrelid, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(relationId)); scan = systable_beginscan(conRel, ConstraintRelidTypidNameIndexId, true, NULL, 1, &key); while (HeapTupleIsValid(tuple = systable_getnext(scan))) deparse_AlterTableStmt() case "add constraint" idx = relation_open(istmt->indexOid, AccessShareLock); ... new_jsonb_VA(state, 7,... "name", jbvString, get_constraint_name(conOid), ... RelationGetRelationName(idx)); relation_close(idx, AccessShareLock); deparse_AlterTableStmt() case "add index" idx = relation_open(idxOid, AccessShareLock); idxname = RelationGetRelationName(idx); constrOid = get_relation_constraint_oid(cmd->d.alterTable.objectId, idxname, false); ... new_jsonb_VA(state, 4,... pg_get_constraintdef_string(constrOid)); ... relation_close(idx, AccessShareLock) [2] table_open(xxRelationId, xxLock); ScanKeyInit(.. systable_endscan relation_close Best Regards, Hou zj