From: Quan Zongliang <quanzongliang@gmail.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Restore replication settings when modifying a field type
Date: Tue, 5 Nov 2019 08:37:17 +0800
Message-ID: <7f042818-cc4f-8d4a-2923-d352f5ed10d6@gmail.com> (raw)
In-Reply-To: <20191028.133939.559712139043653003.horikyota.ntt@gmail.com>
References: <c70fcab2-4866-0d9f-1d01-e75e189db342@gmail.com>
<20191028.133939.559712139043653003.horikyota.ntt@gmail.com>
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 <quanzongliang@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.
>
Attachments:replidfieldtype.patch (text/plain, 6.2K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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.cindex 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.hindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: quanzongliang@gmail.com, horikyota.ntt@gmail.com
Subject: Re: Restore replication settings when modifying a field type
In-Reply-To: <7f042818-cc4f-8d4a-2923-d352f5ed10d6@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox