Re: Support logical replication of DDLs

On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsajin@gmail.com> wrote:
>

Comments on 0001 and 0002
=======================
1.
  * CREATE COLLATION
  */
 ObjectAddress
-DefineCollation(ParseState *pstate, List *names, List *parameters,
bool if_not_exists)
+DefineCollation(ParseState *pstate, List *names, List *parameters,
+ bool if_not_exists, ObjectAddress *from_existing_collid)

I think it is better to expand function header comments to explain
return values especially from_existing_collid.

2.
+Form_pg_sequence_data
+get_sequence_values(Oid sequenceId)
+{
+ Buffer      buf;
+ SeqTable    elm;
+ Relation    seqrel;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+ Form_pg_sequence_data retSeq = palloc(sizeof(FormData_pg_sequence_data));
+
+ /* Open and AccessShareLock sequence */
+ init_sequence(sequenceId, &elm, &seqrel);

The comment above init_sequence() seems wrong to me. AFAICS, we
acquire RowExclusiveLock in init_sequence() via
lock_and_open_sequence(). Am, I missing anything?

3.
+get_sequence_values(Oid sequenceId)
...
...
+
+ if (pg_class_aclcheck(sequenceId, GetUserId(),
+ ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),

Why do we need to check UPDATE privilege just for reading the values?

4. I checked the callers of get_sequence_values and they just need
'last_val' but we still expose all values from Form_pg_sequence_data,
not sure if that is required. In deparse_CreateSeqStmt(), we use it to
append RESTART but the CREATE SEQUENCE syntax in docs doesn't have a
RESTART clause, so I am confused as to why the patch appends the same.
If it is really required then please add the comments for the same.

5. In deparse_CreateSeqStmt() and deparse_ColumnIdentity(), we open
SequenceRelationId, is that really required? Isn't locking the
sequence relation sufficient as is done by get_sequence_values()?
Also, I see that deparse_CreateSeqStmt() opens and locks the sequence
whereas deparse_ColumnIdentity() doesn't do the same. Then, we unlock
the relation in some cases but not in others (like get_sequence_values
uses NoLock whereas others release the lock while closing the rel).

6. In get_sequence_values(), we check the permissions whereas callers
just assume that it is done and don't check it while accessing the
sequence. This makes the code a bit unclear.

7. After seeing the above inconsistencies, I am thinking will it be
better to design get_sequence_values() such that it returns both
sequence parameters and last_val in a structure and the callers use
it. That would bit clean and avoid opening the relation multiple
times.

8.
+/*
+ * Return the given object type as a string.
+ * If isgrant is true, then this function is called
+ * while deparsing GRANT statement and some object
+ * names are replaced.
+ */
+const char *
+stringify_objtype(ObjectType objtype, bool isgrant)

Have an empty line after the Return line. The line length appears too short.

9. Similar to stringify_grant_objtype() and
stringify_adefprivs_objtype(), shall we keep the list of all
unsupported types in stringify_objtype()? That will help us to easily
identify which objects are yet not supported.

10. In pg_get_ruledef_detailed(), the code to form a string for qual
and action is mostly the same as what we have in make_ruledef(). I
think we can extract the common code into a separate function to avoid
missing the code updates at one of the places. I see that
'special_exprkind' is present in one place and not in other, it may be
that over time, we have added new things to deparse_context which
doesn't get updated in the patch. Also, I noticed that for
CMD_NOTHING, the patch just ignores the action whereas the core code
does append the definition. We should check whether such a difference
is required and if so, then add comments for the same.

-- 
With Regards,
Amit Kapila.