RE: Support logical replication of DDLs

On Monday, July 10, 2023 3:22 AM Zheng Li <zhengli10@gmail.com> wrote:
> 
> On Tue, Jun 27, 2023 at 6:16 AM vignesh C <vignesh21@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