Hi, here are my code/test review comments for patch v14-0001.
(the v14-0001 docs review was already previously posted)
======
src/backend/commands/subscriptioncmds.c
parse_subscription_conflict_resolvers:
1.
+ /* Check if the conflict type already exists in the list */
+ if (list_member(SeenTypes, makeString(defel->defname)))
This 'SeenTypes' logic of string comparison of list elements all seems
overkill to me.
There is a known number of conflict types, so why not use a more
efficient bool array:
e.g. bool seen_conflict_type[CONFLICT_NUM_TYPES] = {0};
NOTE - I've already made this change in the nits attachment to
demonstrate that it works fine.
~
2.
+ foreach(lc, stmtresolvers)
+ {
+ DefElem *defel = (DefElem *) lfirst(lc);
There are macros to do this, so you don't need to use lfirst(). e.g.
foreach_ptr?
~
3.
nit (a) - remove excessive blank lines
nit (b) - minor comment reword + periods.
~~~
CreateSubscription:
4.
bits32 supported_opts;
SubOpts opts = {0};
AclResult aclresult;
+ ConflictTypeResolver conflictResolvers[CONFLICT_NUM_TYPES];
nit - Suggest rename to 'conflict_resolvers' to keep similar style to
other variables.
~
5.
nit - add a missing period to some comments.
~~~
AlterSubscription:
6.
nit (a) - remove excessing blank lines.
nit (b) - add missing period in comments.
nit (c) - rename 'conflictResolvers' to 'conflict_resolvers' for
consistent variable style
======
src/backend/parser/gram.y
7.
+ | ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR conflict_type
+ {
+ AlterSubscriptionStmt *n =
+ makeNode(AlterSubscriptionStmt);
+
+ n->kind = ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER;
+ n->subname = $3;
+ n->conflict_type = $8;
+ $$ = (Node *) n;
+ }
Why does this only support resetting resolvers for one conflict type at a time?
~
8.
nit - add whitespace blank line
======
src/backend/replication/logical/conflict.c
9.
Add some static assertions for all of the arrays to ensure everything
is declared as expected.
(I've made this change in the nit attachment)
~~~
10.
+#define CONFLICT_TYPE_MAX_RESOLVERS 4
It's a bit fiddly introducing this constant just for the map
dimensions. You might as well use the already defined
CONFLICT_NUM_RESOLVERS.
Yes, I know they are not exactly the same. But, the extra few ints of
space wasted reusing this is insignificant, and there is no
performance loss in the lookup logic (after my other review comment
later). IMO it is easier to use what you already have instead of
introducing yet another hard-to-explain constant.
(I've made this change in the nit attachment)
~~~
11.
+static const int ConflictTypeResolverMap[][CONFLICT_TYPE_MAX_RESOLVERS] = {
+ [CT_INSERT_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_MISSING] = {CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR, CR_SKIP,
CR_ERROR},
+ [CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR},
+ [CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR}
+};
FYI - There is a subtle quirk (and potential bug) lurking in the way
this map is declared. The array elements for resolvers that are not
defined will default to value 0. So "{CR_SKIP, CR_ERROR}" is
essentially saying "{CR_SKIP, CR_ERROR, 0, 0}", and that means that
the enums for resolvers cannot have a value 0 because then this
ConflictTypeResolverMap would be broken. I see that this was already
handled (enums started at 1) but there was no explanatory comment so
it would be easy to unknowingly break it.
A better way (what I am suggesting here) would be to have a -1 marker
to indicate the end of the list of valid resolvers. This removes any
doubt, and it means the enums can start from 0 as normal, and it means
you can quick-exit from the lookup code (suggested later below) for a
performance gain. Basically, win-win-win.
For example:
* NOTE: -1 is an end marker for the list of valid resolvers for each conflict
* type.
*/
static const int ConflictTypeResolverMap[][CONFLICT_NUM_RESOLVERS+1] = {
[CT_INSERT_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_MISSING] = {CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR, CR_SKIP,
CR_ERROR, -1},
[CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR, -1},
[CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1}
};
(I have made this change already in the nit attachment)
~~~
12.
+/*
+ * Default conflict resolver for each conflict type.
+ */
+static const int ConflictTypeDefaultResolvers[] = {
+ [CT_INSERT_EXISTS] = CR_ERROR,
+ [CT_UPDATE_EXISTS] = CR_ERROR,
+ [CT_UPDATE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE,
+ [CT_UPDATE_MISSING] = CR_SKIP,
+ [CT_DELETE_MISSING] = CR_SKIP,
+ [CT_DELETE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE
+
+};
+
IMO you can throw all this away. Instead, simply rearrange the
resolvers in ConflictTypeResolverMap[] and provide a comment to say
that the resolver at index [0] of the valid resolver lists is the
"default" resolver for each conflict type.
(I have made this change already in the nit attachment)
~~~
SetDefaultResolvers:
13.
+/*
+ * Set default values for CONFLICT RESOLVERS for each conflict type
+ */
+void
+SetDefaultResolvers(ConflictTypeResolver * conflictResolvers)
+{
+ ConflictType type;
+
+ for (type = CT_MIN; type <= CT_MAX; type++)
+ {
+ conflictResolvers[type].conflict_type = ConflictTypeNames[type];
+ conflictResolvers[type].resolver =
+ ConflictResolverNames[ConflictTypeDefaultResolvers[type]];
+ }
+}
nit (a) - remove extra blank line about this function
nit (b) - add a period to the function comment
nit (c) - tidy the param.
nit (d) - use for-loop variable
nit (e) - don't need CT_MIN and CT_MAX
~~~
validate_conflict_type_and_resolver:
14.
AFAIK, you could also break out of the resolver loop early if you hit
an invalid resolver, because that means you have already fallen off
the end of the valid resolvers.
e.g. do this (in conjunction with the other suggested
ConflictTypeResolverMap changes)
if (candidate < 0)
{
/* No more possible resolvers for this conflict type. */
break;
}
~
15.
nit (a) - use for-loop variable
nit (b) - improve the comments a bit
nit (c) - don't need CT_MIN and CT_MAX
nit (d) - don't need CR_MIN and CR_MAX
nit (e) - use loop variable 'i'
nit (f) - add a blank line before the return
nit (g) - remove excessive blank lines in the function (after the return)
~~~
GetAndValidateSubsConflictResolverList:
16.
nit (a) - minor tweak function comment
nit (b) - put the "ConflictTypeResolver *conftyperesolver = NULL" in
lower scope.
nit (c) - tweak comments for periods etc.
~
17.
+ /* Add the conflict type to the list of seen types */
+ conflictTypes = lappend(conflictTypes,
+ makeString((char *) conftyperesolver->conflict_type));
All this messing with lists of strings seems somewhat inefficient. We
already know the enum by this point (e.g.
validate_conflict_type_and_resolver returns it) so the "seen" logic
should be possible with a simple bool array.
(I've made this change in the nits attachment to give an example of
what I mean. It seems to work just fine).
~~~
UpdateSubConflictResolvers:
18.
+ if (HeapTupleIsValid(oldtup))
+ {
+ /* Update the new resolver */
+ values[Anum_pg_subscription_conflict_confrres - 1] =
+ CStringGetTextDatum(conftyperesolver->resolver);
+ replaces[Anum_pg_subscription_conflict_confrres - 1] = true;
+
+ newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_subscription_conflict),
+ values, nulls, replaces);
+ CatalogTupleUpdate(pg_subscription_conflict, &oldtup->t_self, newtup);
+ ReleaseSysCache(oldtup);
+ heap_freetuple(newtup);
+ }
+ else
+ elog(ERROR, "cache lookup failed for table conflict %s for subid %u",
+ conftyperesolver->conflict_type, subid);
I think this might be more easily expressed by reversing the condition:
if (!HeapTupleIsValid(oldtup))
elog(ERROR, "cache lookup failed for table conflict %s for subid %u",
conftyperesolver->conflict_type, subid);
...
~~~
ResetConflictResolver:
19.
+ResetConflictResolver(Oid subid, char *conflict_type)
+{
+ ConflictType idx;
+ ConflictTypeResolver conflictResolver;
No point in declaring the variable as a ConflictTypeResolver because
the code is only interested in the resolver name field.
~
20.
nit (a) - Add period to code comments
nit (b) - Remove excess blank lines
nit (c) - Change 'conflict_type' to the words 'conflict type' in a
couple of comments
~~~
conf_detection_check_prerequisites:
21.
Why do we get this warning repeated when just setting the same
resolver as the conflict already has? Even overwriting the resolver
default does this.
~~~
SetSubConflictResolver:
22.
nit (a) - Add period to function comment.
nit (b) - Tidy the spaces in the param declaration
nit (c) - Add period in code comments.
~~~
RemoveSubscriptionConflictResolvers:
23.
nit (a) - Add period to function comment.
nit (b) - Minor change to comment wording.
nit (c) - Add period code to comment.
======
src/bin/pg_dump/pg_dump.c
SKIP REVIEW
======
src/bin/pg_dump/pg_dump.h
SKIP REVIEW
======
src/bin/psql/describe.c
24.
+ if (pset.sversion >= 180000)
+ appendPQExpBuffer(&buf,
+ ", (SELECT string_agg(confrtype || ' = ' || confrres, ', ') \n"
+ " FROM pg_catalog.pg_subscription_conflict c \n"
+ " WHERE c.confsubid = s.oid) AS \"%s\"\n",
+ gettext_noop("Conflict Resolvers"));
Should this SQL include an ORDER BY so the result is the same each time?
~~~
25. Present the information separately.
The current result is a lot of text, which is not particularly
readable. Even when using the "expanded display" mode.
Name | sub1
Owner | postgres
Enabled | t
Publication | {pub1}
Binary | f
Streaming | off
Two-phase commit | d
Disable on error | f
Origin | any
Password required | t
Run as owner? | f
Failover | f
Synchronous commit | off
Conninfo | dbname=test_pub
Skip LSN | 0/0
Conflict Resolvers | insert_exists = error, update_origin_differs =
apply_remote, update_exists = error, update_missing = skip,
delete_origin_differs = apply_remote, delete_missing = skip
I am thinking perhaps the "Conflict Resolver" information should be
presented separately in a list *below* the normal "describe" output.
Other kinds of describe present information this way too (e.g. the
publication lists tables below).
~
26. Identify what was changed by the user
IMO it may also be useful to know which of the conflict resolvers are
the defaults anyway, versus which have been changed to something else
by the user. If you present the information listed separately like
suggested (above) then there would be room to add this additional
info.
======
.../catalog/pg_subscription_conflict.h
27.
+#ifdef CATALOG_VARLEN /* variable-length fields start here */
+ text confrtype BKI_FORCE_NOT_NULL; /* conflict type */
+ text confrres BKI_FORCE_NOT_NULL; /* conflict resolver */
+#endif
These seem strange field names. What do that mean? e.g. for
'confrtype' what is the 'r'?
======
src/include/nodes/parsenodes.h
28.
+ List *resolvers; /* List of conflict resolvers */
+ char *conflict_type; /* conflict_type to be reset */
} AlterSubscriptionStmt;
That 'resolvers' comment should clarify what kind of a list that is.
e.g. Conflict type enums or strings etc.
~~~
29.
+ ALTER_SUBSCRIPTION_CONFLICT_RESOLVERS,
+ ALTER_SUBSCRIPTION_RESET_ALL_CONFLICT_RESOLVERS,
+ ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER,
} AlterSubscriptionType;
I think these names should mimic the actual syntax more closely.
e.g.
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER,
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER_RESET
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER_RESET_ALL
~~~
30.
+ List *resolvers; /* List of conflict resolvers */
+ char *conflict_type; /* conflict_type to be reset */
} AlterSubscriptionStmt;
30a.
(same as earlier review comment #28)
That 'resolvers' comment should clarify what kind of a list that is.
e.g. Conflict type enums or strings etc.
~
30b.
nit (a) - better name here might be "conflict_type_name", so it is not
confused with the enum
nit (b) - fix the comment: e.g. you can't define 'conflict_type' in
terms of 'conflict_type'
======
src/include/replication/conflict.h
31.
+/* Min and max conflict type */
+#define CT_MIN CT_INSERT_EXISTS
+#define CT_MAX CT_DELETE_MISSING
+
+/*
+ * Conflict resolvers that can be used to resolve various conflicts.
+ *
+ * See ConflictTypeResolverMap in conflict.c to find out which all
+ * resolvers are supported for each conflict type.
+ */
+typedef enum ConflictResolver
+{
+ /* Apply the remote change */
+ CR_APPLY_REMOTE = 1,
nit - these CT_MIN and CT_MAX are unnecessary. If the enums start at 0
(which they do in C by default anyhow) then you only need to know
'CONFLICT_NUM_TYPES'. Code previously using CT_MIN and CT_MAX will
become simpler too.
~~~
32.
+/*
+ * Conflict resolvers that can be used to resolve various conflicts.
+ *
+ * See ConflictTypeResolverMap in conflict.c to find out which all
+ * resolvers are supported for each conflict type.
+ */
+typedef enum ConflictResolver
+{
+ /* Apply the remote change */
+ CR_APPLY_REMOTE = 1,
+
+ /* Keep the local change */
+ CR_KEEP_LOCAL,
+
+ /* Apply the remote change; skip if it can not be applied */
+ CR_APPLY_OR_SKIP,
+
+ /* Apply the remote change; emit error if it can not be applied */
+ CR_APPLY_OR_ERROR,
+
+ /* Skip applying the change */
+ CR_SKIP,
+
+ /* Error out */
+ CR_ERROR,
+} ConflictResolver;
+
+/* Min conflict resolver */
+#define CR_MIN CR_APPLY_REMOTE
+#define CR_MAX CR_ERROR
+
nit (a) - /can not/cannot/
nit (b) - as above, those CR_MIN and CR_MAX are not necessary. It is
simpler to start the enum values at 0 and then you only need to know
CONFLICT_NUM_RESOLVERS. Code previously using CT_MIN and CT_MAX will
become simpler too.
(I made these changes already -- see the attachment).
~~~
33.
+typedef struct ConflictTypeResolver
+{
+ const char *conflict_type;
+ const char *resolver;
+} ConflictTypeResolver;
These seem poor field names that are too easily confused with the
enums etc. Let's name them to make it obvious what they really are:
/conflict_type/conflict_type_name/
/resolver/conflict_resolver_name/
~~~
34.
+extern void SetSubConflictResolver(Oid subId, ConflictTypeResolver *
resolvers, int max_types);
+extern void RemoveSubscriptionConflictById(Oid confid);
+extern void RemoveSubscriptionConflictResolvers(Oid confid);
+extern List *GetAndValidateSubsConflictResolverList(List *stmtresolvers);
+extern void UpdateSubConflictResolvers(List *conflict_resolvers, Oid subid);
+extern ConflictType validate_conflict_type_and_resolver(const char
*conflict_type,
+ const char *conflict_resolver);
+extern void SetDefaultResolvers(ConflictTypeResolver * conflictResolvers);
+extern void ResetConflictResolver(Oid subid, char *conflict_type);
+extern void conf_detection_check_prerequisites(void);
nit (a) - remove some spaces that should not be there
nit (b) - make the parameter names more consistent
======
src/test/regress/sql/subscription.sql
35. General - quotes?
Why are all the resolver names single-quoted? (e.g. "update_missing =
'skip'"). AFAIK it is unnecessary.
~
36. General - why not use describe?
Would the text code be easier if you just validated the changes by
using the "describe" code (e.g. \dRs+ regress_testsub) instead of SQL
select of the pg_subscription_conflict table?
~
37.
nit - The test cases seemed mostly good to me. My nits are only for
small changes/typos or clarifications to the comments.
======
Please see the PS nits attachment. It already implements most of the
nits plus some other review comments were suggested above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 859bf08..703d313 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -450,10 +450,9 @@ parse_subscription_conflict_resolvers(List *stmtresolvers,
ConflictTypeResolver *resolvers)
{
ListCell *lc;
- List *SeenTypes = NIL;
+ bool already_seen[CONFLICT_NUM_TYPES] = {0};
-
- /* First initialize the resolvers with default values. */
+ /* First, initialize the resolvers with default values. */
SetDefaultResolvers(resolvers);
foreach(lc, stmtresolvers)
@@ -462,28 +461,26 @@ parse_subscription_conflict_resolvers(List *stmtresolvers,
ConflictType type;
char *resolver;
- /* Check if the conflict type already exists in the list */
- if (list_member(SeenTypes, makeString(defel->defname)))
- ereport(ERROR,
-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("duplicate conflict type \"%s\"
found", defel->defname)));
-
/* Validate the conflict type and resolver. */
resolver = defGetString(defel);
type = validate_conflict_type_and_resolver(defel->defname,
defGetString(defel));
- /* Add the conflict type to the list of seen types */
- SeenTypes = lappend(SeenTypes, makeString((char
*)resolvers[type].conflict_type));
+ /* Check if the conflict type has been already seen. */
+ if (already_seen[type])
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("duplicate conflict type \"%s\"
found", defel->defname)));
+
+ already_seen[type] = true;
/* Update the corresponding resolver for the given conflict
type. */
- resolvers[type].resolver =
downcase_truncate_identifier(resolver, strlen(resolver), false);
+ resolvers[type].conflict_resolver_name =
downcase_truncate_identifier(resolver, strlen(resolver), false);
}
/* Once validation is complete, warn users if prerequisites are not
met. */
if (stmtresolvers)
conf_detection_check_prerequisites();
-
}
/*
@@ -630,7 +627,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
bits32 supported_opts;
SubOpts opts = {0};
AclResult aclresult;
- ConflictTypeResolver conflictResolvers[CONFLICT_NUM_TYPES];
+ ConflictTypeResolver conflict_resolvers[CONFLICT_NUM_TYPES];
/*
* Parse and check options.
@@ -646,7 +643,7 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
parse_subscription_options(pstate, stmt->options, supported_opts,
&opts);
/* Parse and check conflict resolvers. */
- parse_subscription_conflict_resolvers(stmt->resolvers,
conflictResolvers);
+ parse_subscription_conflict_resolvers(stmt->resolvers,
conflict_resolvers);
/*
* Since creating a replication slot is not transactional, rolling back
@@ -774,8 +771,8 @@ CreateSubscription(ParseState *pstate,
CreateSubscriptionStmt *stmt,
recordDependencyOnOwner(SubscriptionRelationId, subid, owner);
- /* Update the Conflict Resolvers in pg_subscription_conflict */
- SetSubConflictResolver(subid, conflictResolvers, CONFLICT_NUM_TYPES);
+ /* Update the Conflict Resolvers in pg_subscription_conflict. */
+ SetSubConflictResolver(subid, conflict_resolvers, CONFLICT_NUM_TYPES);
ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname,
sizeof(originname));
replorigin_create(originname);
@@ -1639,11 +1636,10 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
{
List *conflict_resolvers = NIL;
-
/* Get the list of conflict types and resolvers
and validate them. */
conflict_resolvers =
GetAndValidateSubsConflictResolverList(stmt->resolvers);
- /* Warn users if prerequisites are not met */
+ /* Warn users if prerequisites are not met. */
conf_detection_check_prerequisites();
/*
@@ -1655,7 +1651,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
}
case ALTER_SUBSCRIPTION_RESET_ALL_CONFLICT_RESOLVERS:
{
- ConflictTypeResolver
conflictResolvers[CONFLICT_NUM_TYPES];
+ ConflictTypeResolver
conflict_resolvers[CONFLICT_NUM_TYPES];
/* Remove the existing conflict resolvers. */
RemoveSubscriptionConflictResolvers(subid);
@@ -1664,8 +1660,8 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
* Create list of conflict resolvers and set
them in the
* catalog.
*/
- SetDefaultResolvers(conflictResolvers);
- SetSubConflictResolver(subid,
conflictResolvers, CONFLICT_NUM_TYPES);
+ SetDefaultResolvers(conflict_resolvers);
+ SetSubConflictResolver(subid,
conflict_resolvers, CONFLICT_NUM_TYPES);
break;
}
case ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER:
@@ -1674,7 +1670,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
* Reset the conflict resolver for this
conflict type to its
* default.
*/
- ResetConflictResolver(subid,
stmt->conflict_type);
+ ResetConflictResolver(subid,
stmt->conflict_type_name);
break;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 8b7023d..e94c91f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10870,10 +10870,11 @@ AlterSubscriptionStmt:
n->kind =
ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER;
n->subname = $3;
- n->conflict_type = $8;
+ n->conflict_type_name = $8;
$$ = (Node *) n;
}
;
+
conflict_type:
Sconst
{ $$ = $1; }
;
diff --git a/src/backend/replication/logical/conflict.c
b/src/backend/replication/logical/conflict.c
index ff44c3c..a75f478 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -40,6 +40,9 @@ static const char *const ConflictTypeNames[] = {
[CT_DELETE_MISSING] = "delete_missing"
};
+StaticAssertDecl(lengthof(ConflictTypeNames) == CONFLICT_NUM_TYPES,
+ "array length mismatch");
+
static const char *const ConflictResolverNames[] = {
[CR_APPLY_REMOTE] = "apply_remote",
[CR_KEEP_LOCAL] = "keep_local",
@@ -49,7 +52,8 @@ static const char *const ConflictResolverNames[] = {
[CR_ERROR] = "error"
};
-#define CONFLICT_TYPE_MAX_RESOLVERS 4
+StaticAssertDecl(lengthof(ConflictResolverNames) == CONFLICT_NUM_RESOLVERS,
+ "array length mismatch");
/*
* Valid conflict resolvers for each conflict type.
@@ -66,28 +70,23 @@ static const char *const ConflictResolverNames[] = {
* Similarly SKIP can be replaced with KEEP_LOCAL for both update_missing
* and delete_missing conflicts. For missing rows, 'SKIP' sounds more user
* friendly name for a resolver and thus has been added here.
+ *
+ * NOTES:
+ * The first resolver (i.e. resolver at index [0]) is the default
+ * resolver for each conflict type. There is a -1 end marker for each list
+ * valid resolvers.
*/
-static const int ConflictTypeResolverMap[][CONFLICT_TYPE_MAX_RESOLVERS] = {
- [CT_INSERT_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
- [CT_UPDATE_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
- [CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
- [CT_UPDATE_MISSING] = {CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR, CR_SKIP,
CR_ERROR},
- [CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR},
- [CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR}
+static const int ConflictTypeResolverMap[][CONFLICT_NUM_RESOLVERS+1] = {
+ [CT_INSERT_EXISTS] = {CR_ERROR, CR_APPLY_REMOTE, CR_KEEP_LOCAL, -1},
+ [CT_UPDATE_EXISTS] = {CR_ERROR, CR_APPLY_REMOTE, CR_KEEP_LOCAL, -1},
+ [CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR,
-1},
+ [CT_UPDATE_MISSING] = {CR_SKIP, CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR,
CR_ERROR, -1},
+ [CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR, -1},
+ [CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR,
-1}
};
-/*
- * Default conflict resolver for each conflict type.
- */
-static const int ConflictTypeDefaultResolvers[] = {
- [CT_INSERT_EXISTS] = CR_ERROR,
- [CT_UPDATE_EXISTS] = CR_ERROR,
- [CT_UPDATE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE,
- [CT_UPDATE_MISSING] = CR_SKIP,
- [CT_DELETE_MISSING] = CR_SKIP,
- [CT_DELETE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE
-
-};
+StaticAssertDecl(lengthof(ConflictTypeResolverMap) == CONFLICT_NUM_TYPES,
+ "array length mismatch");
static int errcode_apply_conflict(ConflictType type);
static int errdetail_apply_conflict(EState *estate,
@@ -547,20 +546,18 @@ build_index_value_desc(EState *estate, Relation localrel,
TupleTableSlot *slot,
return index_value;
}
-
/*
- * Set default values for CONFLICT RESOLVERS for each conflict type
+ * Set default values for CONFLICT RESOLVERS for each conflict type.
*/
void
-SetDefaultResolvers(ConflictTypeResolver * conflictResolvers)
+SetDefaultResolvers(ConflictTypeResolver *resolvers)
{
- ConflictType type;
-
- for (type = CT_MIN; type <= CT_MAX; type++)
+ for (ConflictType type = 0; type < CONFLICT_NUM_TYPES; type++)
{
- conflictResolvers[type].conflict_type = ConflictTypeNames[type];
- conflictResolvers[type].resolver =
-
ConflictResolverNames[ConflictTypeDefaultResolvers[type]];
+ ConflictResolver def_resolver =
ConflictTypeResolverMap[type][0];
+
+ resolvers[type].conflict_type_name = ConflictTypeNames[type];
+ resolvers[type].conflict_resolver_name =
ConflictResolverNames[def_resolver];
}
}
@@ -575,10 +572,9 @@ validate_conflict_type_and_resolver(const char
*conflict_type,
ConflictType type;
ConflictResolver resolver;
bool valid = false;
- int i;
- /* Check conflict type validity */
- for (type = CT_MIN; type <= CT_MAX; type++)
+ /* Validate the conflict type name. */
+ for (type = 0; type < CONFLICT_NUM_TYPES; type++)
{
if (pg_strcasecmp(ConflictTypeNames[type], conflict_type) == 0)
{
@@ -595,8 +591,8 @@ validate_conflict_type_and_resolver(const char
*conflict_type,
/* Reset */
valid = false;
- /* Check conflict resolver validity. */
- for (resolver = CR_MIN; resolver <= CR_MAX; resolver++)
+ /* Validate the conflict resolver name. */
+ for (resolver = 0; resolver < CONFLICT_NUM_RESOLVERS; resolver++)
{
if (pg_strcasecmp(ConflictResolverNames[resolver],
conflict_resolver) == 0)
{
@@ -614,9 +610,17 @@ validate_conflict_type_and_resolver(const char
*conflict_type,
valid = false;
/* Check if conflict resolver is a valid one for the given conflict
type */
- for (i = 0; i < CONFLICT_TYPE_MAX_RESOLVERS; i++)
+ for (int i = 0; i < CONFLICT_NUM_RESOLVERS; i++)
{
- if (ConflictTypeResolverMap[type][i] == resolver)
+ int candidate = ConflictTypeResolverMap[type][i];
+
+ if (candidate < 0)
+ {
+ /* No more possible resolvers for this conflict type. */
+ break;
+ }
+
+ if (candidate == resolver)
{
valid = true;
break;
@@ -629,51 +633,48 @@ validate_conflict_type_and_resolver(const char
*conflict_type,
errmsg("%s is not a valid conflict resolver for
conflict type %s",
conflict_resolver,
conflict_type));
- return type;
+ return type;
}
/*
- * Extract the conflict type and conflict resolvers from the
+ * Extract the conflict types and conflict resolvers from the
* ALTER SUBSCRIPTION command and return a list of ConflictTypeResolver nodes.
*/
List *
GetAndValidateSubsConflictResolverList(List *stmtresolvers)
{
- ConflictTypeResolver *conftyperesolver = NULL;
List *res = NIL;
- List *conflictTypes = NIL;
+ bool already_seen[CONFLICT_NUM_TYPES] = {0};
foreach_ptr(DefElem, defel, stmtresolvers)
{
+ ConflictType conflict_type;
+ ConflictTypeResolver *conftyperesolver;
char *resolver_str;
- /* Check if the conflict type already exists in the list */
- if (list_member(conflictTypes, makeString(defel->defname)))
- {
- ereport(ERROR,
-
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("duplicate conflict type \"%s\"
found", defel->defname)));
- }
-
conftyperesolver = palloc(sizeof(ConflictTypeResolver));
- conftyperesolver->conflict_type =
downcase_truncate_identifier(defel->defname,
+ conftyperesolver->conflict_type_name =
downcase_truncate_identifier(defel->defname,
strlen(defel->defname), false);
resolver_str = defGetString(defel);
- conftyperesolver->resolver =
downcase_truncate_identifier(resolver_str,
+ conftyperesolver->conflict_resolver_name =
downcase_truncate_identifier(resolver_str,
strlen(resolver_str), false);
/*
- * Validate the conflict type and that the resolver is valid
for that
- * conflict type
+ * Validate the conflict type, and check the resolver is valid
for that
+ * conflict type.
*/
- validate_conflict_type_and_resolver(
-
conftyperesolver->conflict_type,
-
conftyperesolver->resolver);
+ conflict_type = validate_conflict_type_and_resolver(
+
conftyperesolver->conflict_type_name,
+
conftyperesolver->conflict_resolver_name);
+
+ /* Check if the conflict type has been seen already. */
+ if (already_seen[conflict_type])
+ ereport(ERROR,
+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("duplicate conflict type \"%s\"
found", conftyperesolver->conflict_type_name)));
- /* Add the conflict type to the list of seen types */
- conflictTypes = lappend(conflictTypes,
-
makeString((char *) conftyperesolver->conflict_type));
+ already_seen[conflict_type] = true;
/* Add the validated ConflictTypeResolver to the result list */
res = lappend(res, conftyperesolver);
@@ -711,7 +712,7 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
/* set up subid and conflict_type to search in cache */
values[Anum_pg_subscription_conflict_confsubid - 1] =
ObjectIdGetDatum(subid);
values[Anum_pg_subscription_conflict_confrtype - 1] =
- CStringGetTextDatum(conftyperesolver->conflict_type);
+
CStringGetTextDatum(conftyperesolver->conflict_type_name);
oldtup = SearchSysCache2(SUBSCRIPTIONCONFLMAP,
values[Anum_pg_subscription_conflict_confsubid - 1],
@@ -721,7 +722,7 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
{
/* Update the new resolver */
values[Anum_pg_subscription_conflict_confrres - 1] =
-
CStringGetTextDatum(conftyperesolver->resolver);
+
CStringGetTextDatum(conftyperesolver->conflict_resolver_name);
replaces[Anum_pg_subscription_conflict_confrres - 1] =
true;
newtup = heap_modify_tuple(oldtup,
RelationGetDescr(pg_subscription_conflict),
@@ -732,7 +733,7 @@ UpdateSubConflictResolvers(List *conflict_resolvers, Oid
subid)
}
else
elog(ERROR, "cache lookup failed for table conflict %s
for subid %u",
- conftyperesolver->conflict_type, subid);
+ conftyperesolver->conflict_type_name, subid);
}
@@ -746,7 +747,7 @@ void
ResetConflictResolver(Oid subid, char *conflict_type)
{
ConflictType idx;
- ConflictTypeResolver conflictResolver;
+ const char *resolver_name;
Datum values[Natts_pg_subscription_conflict];
bool nulls[Natts_pg_subscription_conflict];
bool replaces[Natts_pg_subscription_conflict];
@@ -757,8 +758,8 @@ ResetConflictResolver(Oid subid, char *conflict_type)
char *cur_conflict_res;
Datum datum;
- /* Get the index for this conflict_type */
- for (idx = CT_MIN; idx <= CT_MAX; idx++)
+ /* Get the index for this conflict type. */
+ for (idx = 0; idx < CONFLICT_NUM_TYPES; idx++)
{
if (pg_strcasecmp(ConflictTypeNames[idx], conflict_type) == 0)
{
@@ -772,8 +773,8 @@ ResetConflictResolver(Oid subid, char *conflict_type)
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("%s is not a valid conflict type",
conflict_type));
- /* Get the default resolver for this conflict_type. */
- conflictResolver.resolver =
ConflictResolverNames[ConflictTypeDefaultResolvers[idx]];
+ /* Get the default resolver for this conflict type. */
+ resolver_name = ConflictResolverNames[ConflictTypeResolverMap[idx][0]];
/* Prepare to update a tuple. */
memset(nulls, false, sizeof(nulls));
@@ -789,7 +790,6 @@ ResetConflictResolver(Oid subid, char *conflict_type)
values[Anum_pg_subscription_conflict_confsubid - 1],
values[Anum_pg_subscription_conflict_confrtype - 1]);
-
if (!HeapTupleIsValid(oldtup))
elog(ERROR, "cache lookup failed for table conflict %s for
subid %u", conflict_type, subid);
@@ -798,11 +798,11 @@ ResetConflictResolver(Oid subid, char *conflict_type)
cur_conflict_res = TextDatumGetCString(datum);
/* Check if current resolver is the default one, if not update it. */
- if (pg_strcasecmp(cur_conflict_res, conflictResolver.resolver) != 0)
+ if (pg_strcasecmp(cur_conflict_res, resolver_name) != 0)
{
- /* Update the new resolver */
+ /* Update the new resolver. */
values[Anum_pg_subscription_conflict_confrres - 1] =
- CStringGetTextDatum(conflictResolver.resolver);
+ CStringGetTextDatum(resolver_name);
replaces[Anum_pg_subscription_conflict_confrres - 1] = true;
newtup = heap_modify_tuple(oldtup,
RelationGetDescr(pg_subscription_conflict),
@@ -832,10 +832,10 @@ conf_detection_check_prerequisites(void)
}
/*
- * Set Conflict Resolvers on the subscription
+ * Set Conflict Resolvers on the subscription.
*/
void
-SetSubConflictResolver(Oid subId, ConflictTypeResolver * resolvers, int
resolvers_cnt)
+SetSubConflictResolver(Oid subId, ConflictTypeResolver *resolvers, int
resolvers_cnt)
{
Relation pg_subscription_conflict;
Datum values[Natts_pg_subscription_conflict];
@@ -853,11 +853,11 @@ SetSubConflictResolver(Oid subId, ConflictTypeResolver *
resolvers, int resolver
{
values[Anum_pg_subscription_conflict_confsubid - 1] =
ObjectIdGetDatum(subId);
values[Anum_pg_subscription_conflict_confrtype - 1] =
- CStringGetTextDatum(resolvers[idx].conflict_type);
+ CStringGetTextDatum(resolvers[idx].conflict_type_name);
values[Anum_pg_subscription_conflict_confrres - 1] =
- CStringGetTextDatum(resolvers[idx].resolver);
+
CStringGetTextDatum(resolvers[idx].conflict_resolver_name);
- /* Get a new oid and update the tuple into catalog */
+ /* Get a new oid and update the tuple into catalog. */
conflict_oid = GetNewOidWithIndex(pg_subscription_conflict,
SubscriptionConflictOidIndexId,
Anum_pg_subscription_conflict_oid);
values[Anum_pg_subscription_conflict_oid - 1] =
ObjectIdGetDatum(conflict_oid);
@@ -871,7 +871,7 @@ SetSubConflictResolver(Oid subId, ConflictTypeResolver *
resolvers, int resolver
}
/*
- * Remove the subscription conflict resolvers for the subscription id
+ * Remove the subscription conflict resolvers for the subscription id.
*/
void
RemoveSubscriptionConflictResolvers(Oid subid)
@@ -884,8 +884,8 @@ RemoveSubscriptionConflictResolvers(Oid subid)
rel = table_open(SubscriptionConflictId, RowExclusiveLock);
/*
- * Search using the subid, this should return all conflict resolvers for
- * this sub
+ * Search using the subid to return all conflict resolvers for
+ * this subscription.
*/
ScanKeyInit(&skey[0],
Anum_pg_subscription_conflict_confsubid,
@@ -895,7 +895,7 @@ RemoveSubscriptionConflictResolvers(Oid subid)
scan = table_beginscan_catalog(rel, 1, skey);
- /* Iterate through the tuples and delete them */
+ /* Iterate through the tuples and delete them. */
while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
CatalogTupleDelete(rel, &tup->t_self);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 7e19e0c..0b2832d 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -4228,7 +4228,7 @@ typedef struct AlterSubscriptionStmt
List *publication; /* One or more publication to subscribe
to */
List *options; /* List of DefElem nodes */
List *resolvers; /* List of conflict resolvers */
- char *conflict_type; /* conflict_type to be reset */
+ char *conflict_type_name; /* Name of the conflict type to be
reset */
} AlterSubscriptionStmt;
typedef struct DropSubscriptionStmt
diff --git a/src/include/replication/conflict.h
b/src/include/replication/conflict.h
index fcd49da..c5c3c20 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -50,10 +50,6 @@ typedef enum
#define CONFLICT_NUM_TYPES (CT_DELETE_MISSING + 1)
-/* Min and max conflict type */
-#define CT_MIN CT_INSERT_EXISTS
-#define CT_MAX CT_DELETE_MISSING
-
/*
* Conflict resolvers that can be used to resolve various conflicts.
*
@@ -63,15 +59,15 @@ typedef enum
typedef enum ConflictResolver
{
/* Apply the remote change */
- CR_APPLY_REMOTE = 1,
+ CR_APPLY_REMOTE,
/* Keep the local change */
CR_KEEP_LOCAL,
- /* Apply the remote change; skip if it can not be applied */
+ /* Apply the remote change; skip if it cannot be applied */
CR_APPLY_OR_SKIP,
- /* Apply the remote change; emit error if it can not be applied */
+ /* Apply the remote change; emit error if it cannot be applied */
CR_APPLY_OR_ERROR,
/* Skip applying the change */
@@ -81,14 +77,12 @@ typedef enum ConflictResolver
CR_ERROR,
} ConflictResolver;
-/* Min conflict resolver */
-#define CR_MIN CR_APPLY_REMOTE
-#define CR_MAX CR_ERROR
+#define CONFLICT_NUM_RESOLVERS (CR_ERROR + 1)
typedef struct ConflictTypeResolver
{
- const char *conflict_type;
- const char *resolver;
+ const char *conflict_type_name;
+ const char *conflict_resolver_name;
} ConflictTypeResolver;
extern bool GetTupleTransactionInfo(TupleTableSlot *localslot,
@@ -103,14 +97,14 @@ extern void ReportApplyConflict(EState *estate,
ResultRelInfo *relinfo,
Oid indexoid,
TransactionId localxmin,
RepOriginId
localorigin, TimestampTz localts);
extern void InitConflictIndexes(ResultRelInfo *relInfo);
-extern void SetSubConflictResolver(Oid subId, ConflictTypeResolver *
resolvers, int max_types);
+extern void SetSubConflictResolver(Oid subId, ConflictTypeResolver *resolvers,
int max_types);
extern void RemoveSubscriptionConflictById(Oid confid);
extern void RemoveSubscriptionConflictResolvers(Oid confid);
extern List *GetAndValidateSubsConflictResolverList(List *stmtresolvers);
extern void UpdateSubConflictResolvers(List *conflict_resolvers, Oid subid);
extern ConflictType validate_conflict_type_and_resolver(const char
*conflict_type,
const char *conflict_resolver);
-extern void SetDefaultResolvers(ConflictTypeResolver * conflictResolvers);
+extern void SetDefaultResolvers(ConflictTypeResolver *resolvers);
extern void ResetConflictResolver(Oid subid, char *conflict_type);
extern void conf_detection_check_prerequisites(void);
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index 9fa2a3f..c62c745 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -407,14 +407,14 @@ ERROR: foo is not a valid conflict resolver
-- fail - invalid conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (foo = 'keep_local') WITH (connect =
false);
ERROR: foo is not a valid conflict type
--- fail - invalid resolver for that conflict type
+-- fail - invalid resolver for given conflict type
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub
CONFLICT RESOLVER (update_missing = 'apply_remote') WITH (connect = false);
ERROR: apply_remote is not a valid conflict resolver for conflict type
update_missing
-- fail - duplicate conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
insert_exists = 'keep_local');
ERROR: duplicate conflict type "insert_exists" found
--- creating subscription should create default conflict resolvers
+-- ok - create subscription with no conflict resolvers should create defaults
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false);
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot,
enable the subscription, and refresh the subscription.
@@ -431,13 +431,13 @@ SELECT confrtype, confrres FROM pg_subscription_conflict
ORDER BY confrtype;
ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub;
--- ok - valid conflict type and resolvers
+-- ok - create subscription specifying valid conflict type and resolvers
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
update_missing = 'skip', delete_origin_differs = 'keep_local' ) WITH (connect =
false);
WARNING: conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL: Conflicts update_origin_differs and delete_origin_differs cannot be
detected, and the origin and commit timestamp for the local row will not be
logged.
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot,
enable the subscription, and refresh the subscription.
---check if above are configured; for non specified conflict types, default
resolvers should be seen
+--check if above are configured; for non-specified conflict types, default
resolvers should be seen
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
confrtype | confrres
-----------------------+--------------
@@ -449,16 +449,16 @@ SELECT confrtype, confrres FROM pg_subscription_conflict
ORDER BY confrtype;
update_origin_differs | apply_remote
(6 rows)
--- fail - altering with invalid conflict type
+-- fail - alter with invalid conflict type
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (foo = 'keep_local');
ERROR: foo is not a valid conflict type
--- fail - altering with invalid conflict resolver
+-- fail - alter with invalid conflict resolver
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists = 'foo');
ERROR: foo is not a valid conflict resolver
--- fail - altering with duplicate conflict types
+-- fail - alter with duplicate conflict types
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', insert_exists = 'apply_remote');
ERROR: duplicate conflict type "insert_exists" found
--- ok - valid conflict types and resolvers
+-- ok - alter to set valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', update_missing = 'skip', delete_origin_differs = 'keep_local' );
WARNING: conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL: Conflicts update_origin_differs and delete_origin_differs cannot be
detected, and the origin and commit timestamp for the local row will not be
logged.
@@ -473,7 +473,7 @@ SELECT confrtype, confrres FROM pg_subscription_conflict
ORDER BY confrtype;
update_origin_differs | apply_remote
(6 rows)
--- ok - valid conflict types and resolvers
+-- ok - alter to set valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (update_exists =
'keep_local', delete_missing = 'error', update_origin_differs = 'error');
WARNING: conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp
DETAIL: Conflicts update_origin_differs and delete_origin_differs cannot be
detected, and the origin and commit timestamp for the local row will not be
logged.
@@ -488,10 +488,10 @@ SELECT confrtype, confrres FROM pg_subscription_conflict
ORDER BY confrtype;
update_origin_differs | error
(6 rows)
--- fail - reset with an invalid conflit type
+-- fail - alter to reset an invalid conflict type
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'foo';
ERROR: foo is not a valid conflict type
--- ok - valid conflict type
+-- ok - alter to reset a valid conflict type
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'insert_exists';
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
confrtype | confrres
@@ -504,7 +504,7 @@ SELECT confrtype, confrres FROM pg_subscription_conflict
ORDER BY confrtype;
update_origin_differs | error
(6 rows)
--- ok - reset ALL
+-- ok - alter to reset conflict resolvers for all conflict types
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER ALL;
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
confrtype | confrres
diff --git a/src/test/regress/sql/subscription.sql
b/src/test/regress/sql/subscription.sql
index 596f2e1..b081c1d 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -278,51 +278,50 @@ CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUB
-- fail - invalid conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (foo = 'keep_local') WITH (connect =
false);
--- fail - invalid resolver for that conflict type
+-- fail - invalid resolver for given conflict type
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub
CONFLICT RESOLVER (update_missing = 'apply_remote') WITH (connect = false);
-- fail - duplicate conflict types
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
insert_exists = 'keep_local');
--- creating subscription should create default conflict resolvers
+-- ok - create subscription with no conflict resolvers should create defaults
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub WITH (connect = false);
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
ALTER SUBSCRIPTION regress_testsub SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub;
--- ok - valid conflict type and resolvers
+-- ok - create subscription specifying valid conflict type and resolvers
CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist'
PUBLICATION testpub CONFLICT RESOLVER (insert_exists = 'keep_local',
update_missing = 'skip', delete_origin_differs = 'keep_local' ) WITH (connect =
false);
-
---check if above are configured; for non specified conflict types, default
resolvers should be seen
+--check if above are configured; for non-specified conflict types, default
resolvers should be seen
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
--- fail - altering with invalid conflict type
+-- fail - alter with invalid conflict type
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (foo = 'keep_local');
--- fail - altering with invalid conflict resolver
+-- fail - alter with invalid conflict resolver
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists = 'foo');
--- fail - altering with duplicate conflict types
+-- fail - alter with duplicate conflict types
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', insert_exists = 'apply_remote');
--- ok - valid conflict types and resolvers
+-- ok - alter to set valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (insert_exists =
'apply_remote', update_missing = 'skip', delete_origin_differs = 'keep_local' );
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
--- ok - valid conflict types and resolvers
+-- ok - alter to set valid conflict types and resolvers
ALTER SUBSCRIPTION regress_testsub CONFLICT RESOLVER (update_exists =
'keep_local', delete_missing = 'error', update_origin_differs = 'error');
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
--- fail - reset with an invalid conflit type
+-- fail - alter to reset an invalid conflict type
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'foo';
--- ok - valid conflict type
+-- ok - alter to reset a valid conflict type
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER for 'insert_exists';
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
--- ok - reset ALL
+-- ok - alter to reset conflict resolvers for all conflict types
ALTER SUBSCRIPTION regress_testsub RESET CONFLICT RESOLVER ALL;
SELECT confrtype, confrres FROM pg_subscription_conflict ORDER BY confrtype;
