On 2019/10/28 12:39, Kyotaro Horiguchi wrote:
Hello.
# The patch no longer applies on the current master. Needs a rebasing.
New patch, rebased on master branch.
At Sat, 26 Oct 2019 16:50:48 +0800, Quan Zongliang <quanzongli...@gmail.com>
wrote in
In fact, the replication property of the table has not been modified,
and it is still 'i'(REPLICA_IDENTITY_INDEX). But the previously
specified index property 'indisreplident' is set to false because of
the rebuild.
I suppose that the behavior is intended. Change of column types on the
publisher side can break the agreement on replica identity with
subscribers. Thus replica identity setting cannot be restored
unconditionally. For (somewhat artifitial :p) example:
P=# create table t (c1 integer, c2 text unique not null);
P=# alter table t replica identity using index t_c2_key;
P=# create publication p1 for table t;
P=# insert into t values (0, '00'), (1, '01');
S=# create table t (c1 integer, c2 text unique not null);
S=# alter table t replica identity using index t_c2_key;
S=# create subscription s1 connection '...' publication p1;
Your patch allows change of the type of c2 into integer.
P=# alter table t alter column c2 type integer using c2::integer;
P=# update t set c1 = c1 + 1 where c2 = '01';
This change doesn't affect perhaps as expected.
S=# select * from t;
c1 | c2
----+----
0 | 00
1 | 01
(2 rows)
So I developed a patch. If the user modifies the field type. The
associated index is REPLICA IDENTITY. Rebuild and restore replication
settings.
Explicit setting of replica identity premises that they are sure that
the setting works correctly. Implicit rebuilding after a type change
can silently break it.
At least we need to guarantee that the restored replica identity
setting is truly compatible with all existing subscribers. I'm not
sure about potential subscribers..
Anyway I think it is a problem that replica identity setting is
dropped silently. Perhaps a message something like "REPLICA IDENTITY
setting is lost, please redefine after confirmation of compatibility
with subscribers." is needed.
regards.
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3316..02f8dbeabf 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -174,6 +174,8 @@ typedef struct AlteredTableInfo
List *changedConstraintDefs; /* string definitions of same */
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
+ Oid changedReplIdentOid; /* OID of index to reset
REPLICA IDENTIFY */
+ char *changedReplIdentDef; /* string definitions of same */
} AlteredTableInfo;
/* Struct describing one new constraint to check in Phase 3 scan */
@@ -459,6 +461,7 @@ static ObjectAddress ATExecAlterColumnType(AlteredTableInfo
*tab, Relation rel,
AlterTableCmd *cmd, LOCKMODE lockmode);
static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab);
static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab);
+static void RememberReplicaIdentForRebuilding(Oid indoid, AlteredTableInfo
*tab);
static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
LOCKMODE
lockmode);
static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
@@ -10715,6 +10718,9 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation
rel,
{
Assert(foundObject.objectSubId
== 0);
RememberIndexForRebuilding(foundObject.objectId, tab);
+
+ if
(RelationGetForm(rel)->relreplident==REPLICA_IDENTITY_INDEX)
+
RememberReplicaIdentForRebuilding(foundObject.objectId, tab);
}
else if (relKind == RELKIND_SEQUENCE)
{
@@ -10749,9 +10755,18 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation
rel,
}
case OCLASS_CONSTRAINT:
- Assert(foundObject.objectSubId == 0);
-
RememberConstraintForRebuilding(foundObject.objectId, tab);
- break;
+ {
+ Oid indId;
+
+ Assert(foundObject.objectSubId == 0);
+
RememberConstraintForRebuilding(foundObject.objectId, tab);
+
+ indId =
get_constraint_index(foundObject.objectId);
+ if (OidIsValid(indId))
+
RememberReplicaIdentForRebuilding(indId, tab);
+
+ break;
+ }
case OCLASS_REWRITE:
/* XXX someday see if we can cope with revising
views */
@@ -11075,6 +11090,36 @@ RememberConstraintForRebuilding(Oid conoid,
AlteredTableInfo *tab)
}
}
+/*
+ * Subroutine for ATExecAlterColumnType: remember that a replica identify
+ * needs to be reset (which we might already know).
+ */
+static void
+RememberReplicaIdentForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+ char *defstring;
+
+ /*
+ * This de-duplication check is critical for two independent reasons: we
+ * mustn't try to recreate the same constraint twice, and if a
constraint
+ * depends on more than one column whose type is to be altered, we must
+ * capture its definition string before applying any of the column type
+ * changes. ruleutils.c will get confused if we ask again later.
+ */
+ if (OidIsValid(tab->changedReplIdentOid))
+ return;
+
+ /* OK, capture the constraint's existing definition string */
+ defstring = pg_get_replidentdef_command(indoid);
+
+ /* not a replica identify */
+ if (defstring==NULL)
+ return;
+
+ tab->changedReplIdentOid = indoid;
+ tab->changedReplIdentDef = defstring;
+}
+
/*
* Subroutine for ATExecAlterColumnType: remember that an index needs
* to be rebuilt (which we might already know).
@@ -11222,6 +11267,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo
*tab, LOCKMODE lockmode)
add_exact_object_address(&obj, objects);
}
+ if (OidIsValid(tab->changedReplIdentOid))
+ {
+ Oid relid = IndexGetRelation(tab->changedReplIdentOid, false);
+ ATPostAlterTypeParse(InvalidOid, relid, InvalidOid,
+
tab->changedReplIdentDef,
+ wqueue, lockmode,
tab->rewrite);
+ }
+
+
/*
* It should be okay to use DROP_RESTRICT here, since nothing else
should
* be depending on these objects.
@@ -11373,6 +11427,11 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid
refRelId, char *cmd,
NIL,
con->conname);
}
+ else if (cmd->subtype == AT_ReplicaIdentity)
+ {
+ tab->subcmds[AT_PASS_MISC] =
+
lappend(tab->subcmds[AT_PASS_MISC], cmd);
+ }
else if (cmd->subtype == AT_SetNotNull)
{
/*
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 3e64390d81..c5efe93c0c 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -1854,6 +1854,44 @@ pg_get_partconstrdef_string(Oid partitionId, char
*aliasname)
return deparse_expression((Node *) constr_expr, context, true, false);
}
+/*
+ * pg_get_replidentdef_command
+ *
+ * Returns the definition for the REPLICA IDENTITY, ie, everything that needs
to
+ * appear after "ALTER TABLE ... REPLICA IDENTITY USING INDEX <indexname>".
+ */
+char *pg_get_replidentdef_command(Oid indId)
+{
+ Oid relId;
+ HeapTuple ht_idx;
+ Form_pg_index idxrec;
+
+ StringInfoData buf;
+
+ ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indId));
+ if (!HeapTupleIsValid(ht_idx))
+ elog(ERROR, "cache lookup failed for index %u", indId);
+
+ idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
+ /* not a replica identify */
+ if (!idxrec->indisreplident)
+ {
+ ReleaseSysCache(ht_idx);
+ return NULL;
+ }
+ relId = idxrec->indrelid;
+ ReleaseSysCache(ht_idx);
+
+ initStringInfo(&buf);
+ appendStringInfoString(&buf, "ALTER TABLE ");
+
+ appendStringInfoString(&buf, generate_qualified_relation_name(relId));
+ appendStringInfoString(&buf, " REPLICA IDENTITY USING INDEX ");
+ appendStringInfoString(&buf, quote_identifier(get_rel_name(indId)));
+
+ return buf.data;
+}
+
/*
* pg_get_constraintdef
*
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
index d34cad2f4b..71c3da1316 100644
--- a/src/include/utils/ruleutils.h
+++ b/src/include/utils/ruleutils.h
@@ -24,6 +24,8 @@ extern char *pg_get_indexdef_columns(Oid indexrelid, bool
pretty);
extern char *pg_get_partkeydef_columns(Oid relid, bool pretty);
extern char *pg_get_partconstrdef_string(Oid partitionId, char *aliasname);
+extern char *pg_get_replidentdef_command(Oid indId);
+
extern char *pg_get_constraintdef_command(Oid constraintId);
extern char *deparse_expression(Node *expr, List *dpcontext,
bool
forceprefix, bool showimplicit);
