On Wed, Aug 19, 2020 at 05:33:36PM +0530, Rahila Syed wrote:
> I couldn't test the patch as it does not apply cleanly on master.
The CF bot is green, and I can apply v2 cleanly FWIW:
http://commitfest.cputube.org/michael-paquier.html
> Please find below some review comments:
>
> 1. Would it better to throw a warning at the time of dropping the REPLICA
> IDENTITY index that it would also drop the REPLICA IDENTITY of the
> parent table?
Not sure that's worth it. Updating relreplident is a matter of
consistency.
> 2. CCI is used after calling SetRelationReplIdent from index_drop() but not
> after following call
>
> relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
> bool is_internal)
>
> + /* update relreplident if necessary */
> + SetRelationReplIdent(RelationGetRelid(rel), ri_type);
Yes, not having a CCI here is the intention, because it is not
necessary. That's not the case in index_drop() where the pg_class
entry of the parent table gets updated afterwards.
> 3. I think the former variable names `pg_class_tuple` and
> `pg_class_form`provided better clarity
> + HeapTuple tuple;
>
> + Form_pg_class classtuple;
This is chosen to be consistent with SetRelationHasSubclass().
> 4. I am not aware of the reason of the current behavior, however it seems
> better
>
> to switch to REPLICA_IDENTITY_DEFAULT. Can't think of a reason why user
> would not be fine with using primary key as replica identity when
> replica identity index is dropped
Using NOTHING is more consistent with the current behavior we have
since 9.4 now. There could be an argument for switching back to the
default, but that could be surprising to change an old behavior.
--
Michael
